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

FW-887: four independent fixes for TestBedRender3 #28033

Closed
wants to merge 4 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@alxhub
Copy link
Contributor

alxhub commented Jan 10, 2019

No description provided.

@alxhub alxhub requested review from angular/fw-core as code owners Jan 10, 2019

@googlebot googlebot added the cla: yes label Jan 10, 2019

@alxhub alxhub requested review from jasonaden and kara Jan 10, 2019

@alxhub

This comment has been minimized.

Copy link
Contributor

alxhub commented Jan 10, 2019

Reviewers: please note the last two commits are the substance of this PR. The first commit is from #27860 which I expect to merge before this one, so I've based mine on it to avoid later conflicts.

@mary-poppins

This comment has been minimized.

Copy link

mary-poppins commented Jan 10, 2019

@AndrewKushnir

This comment has been minimized.

Copy link
Contributor

AndrewKushnir commented Jan 10, 2019

@alxhub PR #27860 that you based your PR on is now in master, so you can rebased from master branch. Thank you.

@alxhub alxhub force-pushed the alxhub:jit-ngmodule-compilation branch from 0573fff to e1ba988 Jan 10, 2019

@alxhub

This comment has been minimized.

Copy link
Contributor

alxhub commented Jan 10, 2019

@AndrewKushnir thanks, done.

@mary-poppins

This comment has been minimized.

Copy link

mary-poppins commented Jan 10, 2019

@alxhub alxhub force-pushed the alxhub:jit-ngmodule-compilation branch from e1ba988 to 4c48219 Jan 10, 2019

@mary-poppins

This comment has been minimized.

Copy link

mary-poppins commented Jan 10, 2019

@alxhub alxhub force-pushed the alxhub:jit-ngmodule-compilation branch from 4c48219 to a25022e Jan 10, 2019

@mary-poppins

This comment has been minimized.

Copy link

mary-poppins commented Jan 10, 2019

@alxhub alxhub force-pushed the alxhub:jit-ngmodule-compilation branch from a25022e to 54d3da9 Jan 10, 2019

@mary-poppins

This comment has been minimized.

Copy link

mary-poppins commented Jan 10, 2019

@alxhub alxhub force-pushed the alxhub:jit-ngmodule-compilation branch from 54d3da9 to 3bb4e2f Jan 10, 2019

@mary-poppins

This comment has been minimized.

Copy link

mary-poppins commented Jan 10, 2019

@alxhub alxhub requested review from AndrewKushnir and removed request for kara Jan 10, 2019

alxhub added some commits Jan 10, 2019

fix(ivy): make module registration by id idempotent
An @NgModule with an 'id' property has its type registered in a global map
of modules by id. This happens during compilation of the module.

In Ivy, modules are first compiled when the @NgModule decorator executes.
In tests, they might be passed again through the TestBed's compiler,
resulting in a second compilation and registration.

Before this fix, this second registration would cause an error, as the id
was previously registered. This commit makes the registration idempotent,
so if the same module type is being registered for the same id then no
error is thrown.

Testing strategy: future commits change the way the TestBed compiles
modules, causing tests to become sensitive to this bug if not fixed.
feat(ivy): implement a Compiler for use in TestBedRender3
Previously when testing code injected the Compiler, it received the
top-level Compiler implementation defined in linker/compiler.ts
(and governed by the __PRE_R3__ switch). Code running under the
TestBed, however, should always use a TestBed-aware Compiler
implementation.

This commit adds such an implementation to the TestBedRender3,
which passes compiled modules through the _compileNgModule()
function.

With this change, 3 formerly disabled router integration tests
now pass.

FW-855 #resolve
fix(ivy): TestBed should not clobber compilation of global-scope modules
When an @NgModule decorator executes, the module is added to a queue in
render3/jit/module.ts. Reading an ngComponentDef property causes this queue
to be flushed, ensuring that the component gets the correct module scope
applied.

In before_each.ts, a global beforeEach is added to all Angular tests which
calls TestBed.resetTestingModule() prior to running each test. This in turn
clears the module compilation queue (which is correct behavior, as modules
declared within the test should not leak outside of it via the queue).

So far this is okay. But before the first test runs, the module compilation
queue is full of modules declared in global scope. No definitions have been
read, so no flushes of the queue have been triggered. The global beforeEach
triggers a reset of the queue, aborting all of the in-progress global
compilation, breaking those classes when they're later used in tests.

This commit adds logic to TestBedRender3 to respect the state of the module
queue before the TestBed is first initialized or reset. The queue is flushed
prior to such an operation to ensure global compilation is allowed to finish
properly.

With this fix, a platform-server test now passes (previously the <my-child>
element was not detected as a component, because the encompassing module
never finished compilation.

FW-887 #resolve
fix(ivy): fix invalid provider error messages under TestBed
An @NgModule with invalid provider declarations produces errors under
normal circumstances. However, within the TestBed two small issues with
provider overrides interfered with the correct production of these errors:

1. a 'null' provider object caused a premature crash when the TestBed
   attempted to check for a 'provide' property on it with hasOwnProperty().
2. the array of providers would have an empty override array appended to it
   for each input provider, which would pollute the error messages produced
   down the line.

This commit fixes both of these issues, by 1) checking for null and 2)
filtering out the empty override arrays.

Testing strategy: future commits change the way the TestBed compiles
modules, causing tests to become sensitive to this bug if not fixed.

@alxhub alxhub force-pushed the alxhub:jit-ngmodule-compilation branch from 3bb4e2f to 975257a Jan 10, 2019

@alxhub alxhub changed the title FW-887: two independent fixes for TestBedRender3 FW-887: four independent fixes for TestBedRender3 Jan 10, 2019

@mary-poppins

This comment has been minimized.

Copy link

mary-poppins commented Jan 10, 2019

@AndrewKushnir
Copy link
Contributor

AndrewKushnir left a comment

LGTM, thanks @alxhub!

@alxhub alxhub removed the request for review from jasonaden Jan 11, 2019

AndrewKushnir added a commit that referenced this pull request Jan 11, 2019

fix(ivy): fix invalid provider error messages under TestBed (#28033)
An @NgModule with invalid provider declarations produces errors under
normal circumstances. However, within the TestBed two small issues with
provider overrides interfered with the correct production of these errors:

1. a 'null' provider object caused a premature crash when the TestBed
   attempted to check for a 'provide' property on it with hasOwnProperty().
2. the array of providers would have an empty override array appended to it
   for each input provider, which would pollute the error messages produced
   down the line.

This commit fixes both of these issues, by 1) checking for null and 2)
filtering out the empty override arrays.

Testing strategy: future commits change the way the TestBed compiles
modules, causing tests to become sensitive to this bug if not fixed.

PR Close #28033

AndrewKushnir added a commit that referenced this pull request Jan 11, 2019

feat(ivy): implement a Compiler for use in TestBedRender3 (#28033)
Previously when testing code injected the Compiler, it received the
top-level Compiler implementation defined in linker/compiler.ts
(and governed by the __PRE_R3__ switch). Code running under the
TestBed, however, should always use a TestBed-aware Compiler
implementation.

This commit adds such an implementation to the TestBedRender3,
which passes compiled modules through the _compileNgModule()
function.

With this change, 3 formerly disabled router integration tests
now pass.

FW-855 #resolve

PR Close #28033

AndrewKushnir added a commit that referenced this pull request Jan 11, 2019

fix(ivy): TestBed should not clobber compilation of global-scope modu…
…les (#28033)

When an @NgModule decorator executes, the module is added to a queue in
render3/jit/module.ts. Reading an ngComponentDef property causes this queue
to be flushed, ensuring that the component gets the correct module scope
applied.

In before_each.ts, a global beforeEach is added to all Angular tests which
calls TestBed.resetTestingModule() prior to running each test. This in turn
clears the module compilation queue (which is correct behavior, as modules
declared within the test should not leak outside of it via the queue).

So far this is okay. But before the first test runs, the module compilation
queue is full of modules declared in global scope. No definitions have been
read, so no flushes of the queue have been triggered. The global beforeEach
triggers a reset of the queue, aborting all of the in-progress global
compilation, breaking those classes when they're later used in tests.

This commit adds logic to TestBedRender3 to respect the state of the module
queue before the TestBed is first initialized or reset. The queue is flushed
prior to such an operation to ensure global compilation is allowed to finish
properly.

With this fix, a platform-server test now passes (previously the <my-child>
element was not detected as a component, because the encompassing module
never finished compilation.

FW-887 #resolve

PR Close #28033

kyliau added a commit to kyliau/angular that referenced this pull request Jan 11, 2019

fix(ivy): make module registration by id idempotent (angular#28033)
An @NgModule with an 'id' property has its type registered in a global map
of modules by id. This happens during compilation of the module.

In Ivy, modules are first compiled when the @NgModule decorator executes.
In tests, they might be passed again through the TestBed's compiler,
resulting in a second compilation and registration.

Before this fix, this second registration would cause an error, as the id
was previously registered. This commit makes the registration idempotent,
so if the same module type is being registered for the same id then no
error is thrown.

Testing strategy: future commits change the way the TestBed compiles
modules, causing tests to become sensitive to this bug if not fixed.

PR Close angular#28033

kyliau added a commit to kyliau/angular that referenced this pull request Jan 11, 2019

fix(ivy): fix invalid provider error messages under TestBed (angular#…
…28033)

An @NgModule with invalid provider declarations produces errors under
normal circumstances. However, within the TestBed two small issues with
provider overrides interfered with the correct production of these errors:

1. a 'null' provider object caused a premature crash when the TestBed
   attempted to check for a 'provide' property on it with hasOwnProperty().
2. the array of providers would have an empty override array appended to it
   for each input provider, which would pollute the error messages produced
   down the line.

This commit fixes both of these issues, by 1) checking for null and 2)
filtering out the empty override arrays.

Testing strategy: future commits change the way the TestBed compiles
modules, causing tests to become sensitive to this bug if not fixed.

PR Close angular#28033

kyliau added a commit to kyliau/angular that referenced this pull request Jan 11, 2019

feat(ivy): implement a Compiler for use in TestBedRender3 (angular#28033
)

Previously when testing code injected the Compiler, it received the
top-level Compiler implementation defined in linker/compiler.ts
(and governed by the __PRE_R3__ switch). Code running under the
TestBed, however, should always use a TestBed-aware Compiler
implementation.

This commit adds such an implementation to the TestBedRender3,
which passes compiled modules through the _compileNgModule()
function.

With this change, 3 formerly disabled router integration tests
now pass.

FW-855 #resolve

PR Close angular#28033

kyliau added a commit to kyliau/angular that referenced this pull request Jan 11, 2019

fix(ivy): TestBed should not clobber compilation of global-scope modu…
…les (angular#28033)

When an @NgModule decorator executes, the module is added to a queue in
render3/jit/module.ts. Reading an ngComponentDef property causes this queue
to be flushed, ensuring that the component gets the correct module scope
applied.

In before_each.ts, a global beforeEach is added to all Angular tests which
calls TestBed.resetTestingModule() prior to running each test. This in turn
clears the module compilation queue (which is correct behavior, as modules
declared within the test should not leak outside of it via the queue).

So far this is okay. But before the first test runs, the module compilation
queue is full of modules declared in global scope. No definitions have been
read, so no flushes of the queue have been triggered. The global beforeEach
triggers a reset of the queue, aborting all of the in-progress global
compilation, breaking those classes when they're later used in tests.

This commit adds logic to TestBedRender3 to respect the state of the module
queue before the TestBed is first initialized or reset. The queue is flushed
prior to such an operation to ensure global compilation is allowed to finish
properly.

With this fix, a platform-server test now passes (previously the <my-child>
element was not detected as a component, because the encompassing module
never finished compilation.

FW-887 #resolve

PR Close angular#28033

petebacondarwin added a commit to petebacondarwin/angular that referenced this pull request Jan 14, 2019

fix(ivy): make module registration by id idempotent (angular#28033)
An @NgModule with an 'id' property has its type registered in a global map
of modules by id. This happens during compilation of the module.

In Ivy, modules are first compiled when the @NgModule decorator executes.
In tests, they might be passed again through the TestBed's compiler,
resulting in a second compilation and registration.

Before this fix, this second registration would cause an error, as the id
was previously registered. This commit makes the registration idempotent,
so if the same module type is being registered for the same id then no
error is thrown.

Testing strategy: future commits change the way the TestBed compiles
modules, causing tests to become sensitive to this bug if not fixed.

PR Close angular#28033

petebacondarwin added a commit to petebacondarwin/angular that referenced this pull request Jan 14, 2019

fix(ivy): fix invalid provider error messages under TestBed (angular#…
…28033)

An @NgModule with invalid provider declarations produces errors under
normal circumstances. However, within the TestBed two small issues with
provider overrides interfered with the correct production of these errors:

1. a 'null' provider object caused a premature crash when the TestBed
   attempted to check for a 'provide' property on it with hasOwnProperty().
2. the array of providers would have an empty override array appended to it
   for each input provider, which would pollute the error messages produced
   down the line.

This commit fixes both of these issues, by 1) checking for null and 2)
filtering out the empty override arrays.

Testing strategy: future commits change the way the TestBed compiles
modules, causing tests to become sensitive to this bug if not fixed.

PR Close angular#28033

petebacondarwin added a commit to petebacondarwin/angular that referenced this pull request Jan 14, 2019

feat(ivy): implement a Compiler for use in TestBedRender3 (angular#28033
)

Previously when testing code injected the Compiler, it received the
top-level Compiler implementation defined in linker/compiler.ts
(and governed by the __PRE_R3__ switch). Code running under the
TestBed, however, should always use a TestBed-aware Compiler
implementation.

This commit adds such an implementation to the TestBedRender3,
which passes compiled modules through the _compileNgModule()
function.

With this change, 3 formerly disabled router integration tests
now pass.

FW-855 #resolve

PR Close angular#28033

petebacondarwin added a commit to petebacondarwin/angular that referenced this pull request Jan 14, 2019

fix(ivy): TestBed should not clobber compilation of global-scope modu…
…les (angular#28033)

When an @NgModule decorator executes, the module is added to a queue in
render3/jit/module.ts. Reading an ngComponentDef property causes this queue
to be flushed, ensuring that the component gets the correct module scope
applied.

In before_each.ts, a global beforeEach is added to all Angular tests which
calls TestBed.resetTestingModule() prior to running each test. This in turn
clears the module compilation queue (which is correct behavior, as modules
declared within the test should not leak outside of it via the queue).

So far this is okay. But before the first test runs, the module compilation
queue is full of modules declared in global scope. No definitions have been
read, so no flushes of the queue have been triggered. The global beforeEach
triggers a reset of the queue, aborting all of the in-progress global
compilation, breaking those classes when they're later used in tests.

This commit adds logic to TestBedRender3 to respect the state of the module
queue before the TestBed is first initialized or reset. The queue is flushed
prior to such an operation to ensure global compilation is allowed to finish
properly.

With this fix, a platform-server test now passes (previously the <my-child>
element was not detected as a component, because the encompassing module
never finished compilation.

FW-887 #resolve

PR Close angular#28033
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment