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.

Sign up for GitHub

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

Jump to bottom

Improve AOT unit tests #19996

Closed
wants to merge 2 commits into from
Closed

Improve AOT unit tests #19996

wants to merge 2 commits into from

Conversation

tbosch
Copy link
Contributor

@tbosch tbosch commented Oct 28, 2017

No description provided.

@tbosch
feat(testing): allow to pass in aot summaries also to `TestBed.config…
272c27b 
…ureTestingModule`

Also adds caching for summaries.

Closes angular#19817.
@tbosch tbosch added area: core Issues related to the framework runtime target: major This PR is targeted for the next major release labels Oct 28, 2017
@mary-poppins
Copy link

You can preview 7c769d6 at https://pr19996-7c769d6.ngbuilds.io/.

@Toxicable Toxicable mentioned this pull request Oct 30, 2017
Closed
@tbosch
feat(compiler): introduce TestBed.overrideTemplateUsingTestingModule
f29260f 
This allows to overwrite templates for JIT and AOT components alike.

In contrast to `TestBed.overrideTemplate`, the template is compiled
in the context of the testing module, allowing to use other testing
directives.

Closes angular#19815
@mary-poppins
Copy link

You can preview f29260f at https://pr19996-f29260f.ngbuilds.io/.

vicb
vicb reviewed Oct 31, 2017
View reviewed changes
packages/compiler/src/jit/compiler.ts
}

private _addAotSummaries(fn: () => any[]) {
if (this._addedAotSummaries.has(fn)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want to marked only non-functions as visited ?
Could help with stateful functions ? (not sure about the exact use case for fns)

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh this was existing code... so probably not worth changing the behavior

vicb
vicb suggested changes Oct 31, 2017
View reviewed changes
Copy link
Contributor

@vicb vicb left a comment

Choose a reason for hiding this comment

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

see inline comments

packages/core/src/view/services.ts
@@ -139,10 +143,15 @@ function debugCreateEmbeddedView(

function debugCreateComponentView(
parentView: ViewData, nodeDef: NodeDef, viewDef: ViewDefinition, hostElement: any): ViewData {
const defWithOverride = applyProviderOverridesToView(viewDef);
const overrideComponentView =
Copy link
Contributor

Choose a reason for hiding this comment

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

override+n+CV ?

Copy link
Contributor

Choose a reason for hiding this comment

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

we already have OverrideProvider, keeping for consistency

packages/core/test/linker/jit_summaries_integration_spec.ts
it('should allow to override a template', () => {
resetTestEnvironmentWithSummaries(summaries);

TestBed.overrideTemplateUsingTestingModule(SomePublicComponent, 'overwritten');
Copy link
Contributor

Choose a reason for hiding this comment

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

wouldn't this test work the same with overrideTemplate only ?

packages/core/test/linker/jit_summaries_integration_spec.ts

const overwrittenValue = {};

TestBed.overrideProvider(SomeDep, {useFactory: () => overwrittenValue, deps: []});
Copy link
Contributor

Choose a reason for hiding this comment

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

chain call (or assert the type of the return value) ?

packages/core/testing/src/test_bed.ts
@@ -142,9 +142,23 @@ export class TestBed implements Injector {
return TestBed;
}

/**
* Overrides the template of the given component, compiling the template
Copy link
Contributor

Choose a reason for hiding this comment

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

Add docs to overrideTemplate to clearly describe the difference ?

packages/core/testing/src/test_bed.ts
overrideTemplateUsingTestingModule(component: Type<any>, template: string) {
this._assertNotInstantiated('overrideTemplateUsingTestingModule', 'override template');

@Component({selector: 'empty', template: template})
Copy link
Contributor

Choose a reason for hiding this comment

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

nit template: template could be simplified

packages/platform-browser/test/testing_public_spec.ts
/Cannot override template when the test module has already been instantiated/);
});

it('should reset overrides when the testing modules is resetted', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

testing module-s-

@tbosch
Copy link
Contributor Author

tbosch commented Oct 31, 2017

FYI: @vicb is taking over this PR and landing it once Angular 5 is done.

@tbosch
Copy link
Contributor Author

tbosch commented Nov 1, 2017

@vicb will take over this PR.

This was referenced Nov 1, 2017
Closed
Closed
@IgorMinar IgorMinar added target: patch This PR is targeted for the next patch release and removed target: major This PR is targeted for the next major release labels Nov 3, 2017
vicb added a commit to vicb/angular that referenced this pull request Nov 6, 2017
@vicb
refactor(core): misc changes and integrate review feedback on angular…
b62752d 
…#19996
@vicb vicb mentioned this pull request Nov 6, 2017
Closed
vicb added a commit to vicb/angular that referenced this pull request Nov 6, 2017
@vicb
refactor(core): misc changes and integrate review feedback on angular…
fc7aff8 
…#19996
vicb added a commit to vicb/angular that referenced this pull request Nov 6, 2017
@vicb
refactor(core): misc changes and integrate review feedback on angular…
99c4a1b 
…#19996
vicb added a commit to vicb/angular that referenced this pull request Nov 6, 2017
@vicb
refactor(core): misc changes and integrate review feedback on angular…
817d086 
…#19996
vicb added a commit to vicb/angular that referenced this pull request Nov 6, 2017
@vicb
refactor(core): misc changes and integrate review feedback on angular…
fccddae 
…#19996
vicb added a commit that referenced this pull request Nov 6, 2017
@vicb
refactor(core): misc changes and integrate review feedback on #19996
6e8e3bd 
closes #20224
@IgorMinar
Copy link
Contributor

closing in favor of #20224

@IgorMinar IgorMinar closed this Nov 17, 2017
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 12, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@vicb vicb vicb requested changes

@vikerman vikerman Awaiting requested review from vikerman

@chuckjaz chuckjaz Awaiting requested review from chuckjaz

Assignees
No one assigned
Labels
area: core Issues related to the framework runtime cla: yes target: patch This PR is targeted for the next patch release type: bug/fix
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@tbosch @mary-poppins @IgorMinar @vicb @chuckjaz @googlebot