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

feat(ivy): implement TestBed #25369

Closed
wants to merge 7 commits into from
Closed

feat(ivy): implement TestBed #25369

wants to merge 7 commits into from

Conversation

vicb
Copy link
Contributor

@vicb vicb commented Aug 7, 2018

The goal of this PR is to implement the TestBed for IVY so that tests relying on it could be run either for the ViewEngine or for the Render3 without having to modify the test file.

  • Use bazel test <target> to execute a test targeting the ViewEngine
  • Use bazel test <target> --define=compile=jit to execute a test targeting the Render3

This PR also add a spec for the TestBed itself which can be run for either the ViewEngine or Render3:

  • bazel test packages/core/test:test runs the ViewEngine tests
  • bazel test packages/core/test:test --define=compile=jit runs the Render3 tests

Note that in the second case (Render3) there will be quite some errors due to the fact that much more specs than the TestBed spec are executed. You might want to fdescribe the TestBed spec for now when adding functionality.

Notes

  • TestBed now point to either _Render3TestBed or _ViewEngineTestBed depending on if IVY is respectively enabled (--define=compile=jit) or not.
  • _Render3TestBed and _ViewEngineTestBed do not implement a common interface / extends a base class because it would be hard to do with the static methods. However they have a compatible interface and the code would not compile w/o errors if it was not the case.

Todo

see #25389

Recommended usage

Because of the high number of failing tests using IVY, the best for now is to fdescribe a test suite (ie NgFor spec and execute the spec against IVY bazel test packages/common/test:test[_web] and fix the errors by

  • fixing bugs in IVY,
  • implementing missing features in IVY,
  • fixing the _Render3TestBed, DebugContext or DebugNode

@vicb vicb added action: review The PR is still awaiting reviews from at least one requested reviewer comp: ivy and removed cla: yes labels Aug 7, 2018
@vicb vicb force-pushed the 07-r3tb branch 3 times, most recently from ff03264 to ecb9d18 Compare August 8, 2018 03:14
@vicb vicb changed the title feat(ivy): implement TestBed [WIP] feat(ivy): implement TestBed Aug 8, 2018
@vicb vicb added the state: WIP label Aug 8, 2018
@vicb vicb mentioned this pull request Aug 8, 2018
4 tasks
@@ -201,6 +201,11 @@ export function getCreationMode(): boolean {
*/
let viewData: LViewData;

export function getViewData(): LViewData {
// top level variables should not be exported for performance reasons (PERF_NOTES.md)
return viewData;
Copy link
Member

Choose a reason for hiding this comment

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

This is exported already through _getViewData

@vicb vicb force-pushed the 07-r3tb branch 2 times, most recently from 261b70d to ba51342 Compare August 8, 2018 23:03
mhevery
mhevery previously requested changes Aug 9, 2018
const isInternalRootView = rootSelectorOrNode === undefined;

const rendererFactory =
ngModule ? ngModule.injector.get(RendererFactory2) : domRendererFactory3;
(ngModule ? wrapRendererFactory2(ngModule.injector.get(RendererFactory2)) :
Copy link
Contributor

Choose a reason for hiding this comment

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

This needs be done in a way where we don't pull in the TestBed codebase into production application.

const wrapper = ngModule.injector.get(WraperToken, Optional);
let render2 = ngModule.injector.get(RendererFactory2);
if (wrapper) {
  renderer2 = wrapper(render2);
}

@@ -31,13 +30,12 @@ declare global {
rendererRemoveNode: number;
rendererCreateComment: number;
}
const ngDebugRenderer: boolean;
Copy link
Contributor

Choose a reason for hiding this comment

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

revert whole file.

@@ -0,0 +1,258 @@
/**
Copy link
Contributor

Choose a reason for hiding this comment

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

move to @angular/core/testing.

/**
* Base interface to resolve `@Component`, `@Directive`, `@Pipe` and `@NgModule`.
*/
export interface Resolver<T> { resolve(type: Type<any>): T|null; }
Copy link
Contributor

Choose a reason for hiding this comment

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

move to @angular/core/testing

export function defineNgModule({bootstrap}: {bootstrap?: Type<any>[]}):
NgModuleDef<any, any, any, any> {
return ({ bootstrap: bootstrap || [], } as any);
if (ivyEnabled) {
Copy link
Contributor

Choose a reason for hiding this comment

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

change to ivyEnabled && describe(... so that we don't have every line changed by you.

export declare function resetFakeAsyncZone(): void;

export declare class TestBed implements Injector {
export declare class _ViewEngineTestBed implements Injector {
Copy link
Contributor

Choose a reason for hiding this comment

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

Change name to TestBedViewEngine or TestBed2 so that order is preserved.

/** @experimental */
export declare function resetFakeAsyncZone(): void;

export declare const TestBed: typeof _Render3TestBed | typeof _ViewEngineTestBed;
Copy link
Contributor

Choose a reason for hiding this comment

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

_Render3TestBed seems to be missing from export. Also rename to TestBed3

@vicb vicb force-pushed the 07-r3tb branch 7 times, most recently from 5713bb5 to 9677059 Compare August 9, 2018 23:33
@vicb
Copy link
Contributor Author

vicb commented Aug 9, 2018

@vicb vicb changed the title [WIP] feat(ivy): implement TestBed feat(ivy): implement TestBed Aug 9, 2018
@vicb vicb added feature Issue that requests a new feature target: major This PR is targeted for the next major release and removed state: WIP labels Aug 9, 2018
@vicb vicb force-pushed the 07-r3tb branch 2 times, most recently from 86b0064 to 89aed19 Compare August 10, 2018 05:49
@angular angular deleted a comment from mary-poppins Aug 10, 2018
@gregmagolan
Copy link
Contributor

gregmagolan commented Aug 10, 2018

@vicb I looked at the failing integration/bazel test and it looks like the @angular/core bundle is now depending on the @angular/compiler bundle which it wasn't before.

(function (global, factory) {
    typeof exports === 'object' && typeof module !== 'undefined' ? factory(exports, require('rxjs'), require('rxjs/operators'), require('@angular/compiler')) :
    typeof define === 'function' && define.amd ? define('@angular/core', ['exports', 'rxjs', 'rxjs/operators', '@angular/compiler'], factory) :
    (factory((global.ng = global.ng || {}, global.ng.core = {}),global.rxjs,global.rxjs.operators,global.ng.compiler));

That bundle is not included in the devserver since it was an AOT build. Is @angular/core now supposed to depend on @angular/compiler?

@gregmagolan
Copy link
Contributor

gregmagolan commented Aug 10, 2018

The bundles included in the devserver in the integration/bazel test are defined in integration/bazel/BUILD.bazel:

ANGULAR_TESTING = [
    "node_modules/@angular/*/bundles/*-testing.umd.js",
    # We use AOT, so the compiler and the dynamic platform-browser should be
    # visible only in tests
    "node_modules/@angular/compiler/bundles/*.umd.js",
    "node_modules/@angular/platform-browser-dynamic/bundles/*.umd.js",
]

filegroup(
    name = "angular_bundles",
    srcs = glob(
        ["node_modules/@angular/*/bundles/*.umd.js"],
        exclude = ANGULAR_TESTING,
    ),
)

@gregmagolan
Copy link
Contributor

For reference, the core bundle on angular master master looks like this:

(function (global, factory) {
    typeof exports === 'object' && typeof module !== 'undefined' ? factory(exports, require('rxjs'), require('rxjs/operators')) :
    typeof define === 'function' && define.amd ? define('@angular/core', ['exports', 'rxjs', 'rxjs/operators'], factory) :
    (factory((global.ng = global.ng || {}, global.ng.core = {}),global.rxjs,global.rxjs.operators));
``` (only depending on rxjs)

@mary-poppins
Copy link

You can preview 89f7b91 at https://pr25369-89f7b91.ngbuilds.io/.

@mary-poppins
Copy link

You can preview bb6fc89 at https://pr25369-bb6fc89.ngbuilds.io/.

@mary-poppins
Copy link

You can preview d2d5100 at https://pr25369-d2d5100.ngbuilds.io/.

@mary-poppins
Copy link

You can preview 106703a at https://pr25369-106703a.ngbuilds.io/.

@mary-poppins
Copy link

You can preview d084e80 at https://pr25369-d084e80.ngbuilds.io/.

@vicb vicb dismissed mhevery’s stale review August 14, 2018 04:05

@mhevery approved the changed (which apparently do not dismiss prior "requesting changes")

@vicb vicb added action: merge The PR is ready for merge by the caretaker and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Aug 14, 2018
@vicb
Copy link
Contributor Author

vicb commented Aug 14, 2018

Waiting for g3 to go green before re-presubmitting.

@vicb
Copy link
Contributor Author

vicb commented Aug 14, 2018

@mary-poppins
Copy link

You can preview 460d984 at https://pr25369-460d984.ngbuilds.io/.

@benlesh benlesh closed this in 8510637 Aug 14, 2018
benlesh pushed a commit that referenced this pull request Aug 14, 2018
FrederikSchlemmer pushed a commit to FrederikSchlemmer/angular that referenced this pull request Jan 3, 2019
FrederikSchlemmer pushed a commit to FrederikSchlemmer/angular that referenced this pull request Jan 3, 2019
@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 13, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker cla: yes feature Issue that requests a new feature target: major This PR is targeted for the next major release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants