Skip to content
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

UIP-2413: Get tests passing with the DDC #86

Merged
merged 5 commits into from Jun 27, 2017

Conversation

jacehensley-wf
Copy link
Contributor

@jacehensley-wf jacehensley-wf commented Jun 22, 2017

Dependent on #83 and Workiva/react-dart#122

Ultimate problem:

Tests were not passing with the DDC

How it was fixed:

  • Update errors that came up from running the tests with the DDC

Testing suggestions:

  • Verify tests by:
    • Run pub serve test --web-compiler=dartdevc in one terminal
    • And pub run test -p vm -p chrome -x no-ddc --pub-serve=8080 test/vm_tests/ test/over_react_component_declaration_test.dart test/over_react_component_test.dart test/over_react_util_test.dart in another terminal

Potential areas of regression:

N/A


FYA: @greglittlefield-wf @aaronlademann-wf @jacehensley-wf @clairesarsam-wf @joelleibow-wf

@aviary2-wf
Copy link

Raven

Number of Findings: 0

domClassMirror = reflectClass(Dom);

// staticMembers is not implemented for the DDC and will throw is this test is loaded even if it's not run.
try {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We shouldn't need this try-catch, then, if it's not run in the DDC, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It throws even if the test isn't run 😢

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wait, the contents of setUpAll are evaluated even when the group is disabled? Is that a test package bug?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know if it's evaluated, I think the DDC might just be checking for instances of that being used, because it throws if it's in a test too

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But if I use skip: it's fine. So maybe it is a test package bug.

@@ -57,5 +62,5 @@ main() {
expect(component.type, equalsIgnoringCase(expectedTagName));
});
}
});
}, tags: 'no-ddc');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this disabled in the DDC? Should have a comment

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't use staticMethod. I'll add a comment

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, we should really still test these in the DDC if possible.

What about if we enumerated these, and just checked them against the reflected values, like so?

const expectedStaticMethods = const <Symbol> [#div, #span, ...];
var actualMethods;
try {
  actualMethods = domClassMirror.staticMembers.values.map((m) => m.simpleName).toList();
} catch(e) {}
if (actualMethods != null) {
  expect(actualMethods, unorderedEquals(expectedStaticMethods),
      reason: '`expectedStaticMethods` needs to be updated');
}

for (var method in expectedStaticMethods) {
  String name = MirrorSystem.getName(method);
...

@@ -437,6 +437,6 @@ void main() {

group('common component functionality:', () {
commonComponentTests(ResizeSensor);
});
}, tags: 'no-ddc');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this disabled in the DDC? Should have a comment

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't use instanceMembers in the DDC. I'll add a comment

@@ -126,7 +126,7 @@ void mapProxyTests(Map mapProxyFactory(Map proxiedMap)) {
expect(proxy.values, equals(['value']));
verify(mock.values);
});
});
}, tags: 'no-ddc');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this disabled in the DDC? Should have a comment

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mocks don't work in the DDC. I'll add a comment

@@ -73,7 +73,7 @@ main() {
expect(syntheticKeyboardEvent.metaKey, isFalse);
expect(syntheticKeyboardEvent.repeat, isFalse);
expect(syntheticKeyboardEvent.shiftKey, isFalse);
});
}, tags: 'no-ddc');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this disabled in the DDC? Should have a comment

@@ -126,7 +126,7 @@ main() {
expect(syntheticMouseEvent.screenX, isNull);
expect(syntheticMouseEvent.screenY, isNull);
expect(syntheticMouseEvent.shiftKey, isFalse);
});
}, tags: 'no-ddc');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this disabled in the DDC? Should have a comment

@@ -22,12 +22,6 @@ import '../../../test_util/test_util.dart';

main() {
group('transformed component integration:', () {
test('props class cannot be instantiated directly', () {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why was this test removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Prop classes are made abstract by the transformer and the DDC was failing on instantiating an abstract object.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And this test was leftover from before that was the case, so it's not really necessary anymore

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@codecov-io
Copy link

Codecov Report

Merging #86 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master      #86   +/-   ##
=======================================
  Coverage   94.81%   94.81%           
=======================================
  Files          31       31           
  Lines        1522     1522           
=======================================
  Hits         1443     1443           
  Misses         79       79

@aaronlademann-wf
Copy link
Contributor

+1

@greglittlefield-wf
Copy link
Contributor

+1

@leviwith-wf
Copy link
Contributor

QA +10

  • Testing instruction
  • Dev +1's
  • Dev/QA +10
  • Unit tests created/updated
  • All unit tests pass
  • Rosie ran/Rosie comment displays expected info
  • Dependency Scan Clean

Merging.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants