-
Notifications
You must be signed in to change notification settings - Fork 58
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
AF-2799: Remove mirrors from tests #219
Changes from all commits
38e72b9
9241198
1575d1d
4243465
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -17,29 +17,6 @@ library mock_classes; | |
import 'dart:async'; | ||
import 'dart:html'; | ||
|
||
// Tell dart2js that the `mockito` package only needs to reflect the specified mock/spied types. | ||
// This speeds up compilation and makes JS output much smaller. | ||
@MirrorsUsed(targets: const [ | ||
'dart.async.Timer', | ||
'MockTimer', | ||
// Also include Mock classes we use from w_test_tools. | ||
'dart.dom.html.KeyEvent', | ||
'dart.dom.html.HtmlDocument', | ||
'w_test_tools.src.mock_classes.MockKeyEvent', | ||
'w_test_tools.src.mock_classes.MockDocument', | ||
'MockFileList', | ||
'MockFile', | ||
'MockFileUploadInputElement', | ||
'dart.dom.html.FileList', | ||
'dart.dom.html.File', | ||
'dart.dom.html.FileUploadInputElement', | ||
'MockSyntheticEvent', | ||
'MockSyntheticMouseEvent', | ||
'react.SyntheticEvent', | ||
'react.SyntheticMouseEvent', | ||
], override: 'mockito') | ||
import 'dart:mirrors'; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @greglittlefield-wf were these needed? afaik, the only usage of mirrors within mockito was There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Mm, looks like these were added under the false assumption that Away with them! 😄 |
||
|
||
import 'package:mockito/mockito.dart'; | ||
import 'package:react/react.dart' as react; | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -14,35 +14,13 @@ | |
|
||
library test_util.prop_utils; | ||
|
||
// Tell dart2js that this library only needs to reflect types annotated with `Props`/`PropsMixin`. | ||
// This speeds up compilation and makes JS output much smaller. | ||
@MirrorsUsed(metaTargets: const [ | ||
'over_react.component_declaration.annotations.Props', | ||
'over_react.component_declaration.annotations.PropsMixin', | ||
]) | ||
import 'dart:mirrors'; | ||
|
||
import 'package:over_react_test/over_react_test.dart'; | ||
import 'package:test/test.dart'; | ||
|
||
void testProp(Symbol name, dynamic expectedKey, instance, testValue) { | ||
InstanceMirror mirror = reflect(instance); | ||
|
||
mirror.setField(name, testValue); | ||
expect(instance[expectedKey], equals(testValue)); | ||
expect(mirror.getField(name).reflectee, equals(testValue)); | ||
} | ||
|
||
void testKeys(List<String> keys, dynamic instanceBuilder()) { | ||
void testInvalidKey(dynamic instanceBuilder()) { | ||
test('cannot set / read values that are not its prop map', () { | ||
var instance = instanceBuilder(); | ||
expect(() {instance['notThere'];}, | ||
throwsA(hasToStringValue(contains('Map does not contain this key')))); | ||
}); | ||
for (var propKey in keys) { | ||
test('prop: $propKey can have its value set / read', () { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These tests were only intended to verify that the code generation for implementing concrete getters/setters was working properly, and that these props classes had a prop key namespace of |
||
var instance = instanceBuilder(); | ||
testProp(new Symbol(propKey), propKey, instance, null); | ||
}); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using
.toString()
inside a string interpolation is redundant:There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤦♂️