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

refactor(ivy): move di tests for Attribute and special tokens to acceptance #29299

Open
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@cexbrayat
Copy link
Contributor

cexbrayat commented Mar 14, 2019

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • angular.io application / infrastructure changes
  • Other... Please describe:

What is the new behavior?

This moves the @Attribute tests from di_spec.ts (that use manually written Ivy instructions) to the acceptance tests (that use proper templates).

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

I talked about this task with @kara in #29257 I'll start moving the other tests in di_spec.ts when this is merged.
cc @petebacondarwin as he wrote most of these tests

@cexbrayat cexbrayat requested a review from angular/fw-core as a code owner Mar 14, 2019

@googlebot googlebot added the cla: yes label Mar 14, 2019

@petebacondarwin

This comment has been minimized.

Copy link
Member

petebacondarwin commented Mar 14, 2019

I don't think I wrote them :-) I just tweaked them most recently.

@petebacondarwin

This comment has been minimized.

Copy link
Member

petebacondarwin commented Mar 14, 2019

Not sure if this is a flake or a legit failure:

IE 10.0.0 (Windows 8.0.0) DeprecatedDatePipe supports should support numeric strings FAILED
	Expected function not to throw, but it threw SyntaxError: Syntax error in regular expression.
	    at <Jasmine>
	   at Anonymous function (Unknown script code:45:64)
	   at invoke (node_modules/zone.js/dist/zone.js:386:13)
	   at onInvoke (node_modules/zone.js/dist/zone-testing.js:288:13)
	   at invoke (node_modules/zone.js/dist/zone.js:386:13)
	   at run (node_modules/zone.js/dist/zone.js:138:17)
	   at runInTestZone (node_modules/zone.js/dist/zone-testing.js:509:13)
	   at Anonymous function (node_modules/zone.js/dist/zone-testing.js:524:13)
	    at <Jasmine>
	   at invokeTask (node_modules/zone.js/dist/zone.js:419:13)
	   at runTask (node_modules/zone.js/dist/zone.js:188:21)
	   at drainMicroTaskQueue (node_modules/zone.js/dist/zone.js:595:25)
	   at run (node_modules/core-js/client/core.js:6252:13)
	   at Anonymous function (node_modules/core-js/client/core.js:6269:30)
	   at flush (node_modules/core-js/client/core.js:2471:9)

I restarted it to see.

@cexbrayat

This comment has been minimized.

Copy link
Contributor Author

cexbrayat commented Mar 14, 2019

@petebacondarwin Looks like it was a flake, thanks for restarting 👍

@cexbrayat cexbrayat force-pushed the cexbrayat:chore/move-di-tests-to-acceptance branch from a39645c to fec5227 Mar 19, 2019

@cexbrayat cexbrayat changed the title refactor(ivy): move di tests for Attribute to acceptance refactor(ivy): move di tests for Attribute and special tokens to acceptance Mar 19, 2019

@cexbrayat

This comment has been minimized.

Copy link
Contributor Author

cexbrayat commented Mar 19, 2019

@kara I need your advice.

I migrated a few more tests from render3/di_spec.ts to acceptance/di_spec.ts.
As you can see in the CI results, some migrated tests are OK in aot/Ivy mode but not otherwise.

For example, the assertion that each TemplateRef instance should be unique here is OK in Ivy but not otherwise. Same thing with ViewContainerRef.

Is it normal and I should change the test to modifiedInIvy (or something like that), or is it a bug?

@kara

This comment has been minimized.

Copy link
Contributor

kara commented Mar 19, 2019

@cexbrayat Yep, those are expected changes. The tests checking uniqueness should be marked onlyInIvy, since the behavior doesn't occur in View Engine.

@cexbrayat cexbrayat force-pushed the cexbrayat:chore/move-di-tests-to-acceptance branch 2 times, most recently from 9c27dbe to d5f29f8 Mar 19, 2019

cexbrayat added some commits Mar 13, 2019

refactor(ivy): move di test for DI tokens to acceptance
Move tests for special tokens like `Injector`, `ElementRef`, `TemplateRef`, `ViewContainerRef`, `ChangeDectetorRef` and custom string tokens.

@cexbrayat cexbrayat force-pushed the cexbrayat:chore/move-di-tests-to-acceptance branch from d5f29f8 to 87c51ea Mar 20, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.