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

feat(testing): use zones to avoid the need for injectAsync #5375

Closed
wants to merge 1 commit into from
Closed

feat(testing): use zones to avoid the need for injectAsync #5375

wants to merge 1 commit into from

Conversation

juliemr
Copy link
Member

@juliemr juliemr commented Nov 19, 2015

Use a zone counting timeouts and microtasks to determine when a test
is finished, instead of requiring the test writer to use
injectAsync and return a promise.

See #5322

@juliemr juliemr added the area: testing Issues related to Angular testing features, such as TestBed label Nov 19, 2015
@juliemr
Copy link
Member Author

juliemr commented Nov 19, 2015

cc @vicb @IgorMinar

Zones seem to work smoothly and remove complexity for the user for the async tests. All tests which use inject are now being run asynchronously, which as we discussed before adds a very small (couple ms) penalty time to each test. I like this overall - thoughts?

@juliemr juliemr added the action: review The PR is still awaiting reviews from at least one requested reviewer label Nov 19, 2015
@juliemr juliemr added this to the beta-00 milestone Nov 19, 2015
@juliemr
Copy link
Member Author

juliemr commented Nov 19, 2015

Note that this isn't actually a breaking change, because both inject and injectAsync do the same thing now and the return value doesn't matter. A cleanup would remove injectAsync entirely.

@0x-r4bbit
Copy link
Contributor

This is awesome Julie!

On Thu, Nov 19, 2015, 3:52 AM Julie Ralph notifications@github.com wrote:

Note that this isn't actually a breaking change, because both inject and
injectAsync do the same thing now and the return value doesn't matter. A
cleanup would remove injectAsync entirely.

—
Reply to this email directly or view it on GitHub
#5375 (comment).

vicb
vicb reviewed Nov 19, 2015
View reviewed changes
modules/angular2/src/testing/testing.ts
var pendingMicrotasks = 0;
var pendingTimeouts = [];

var ngTestZone = (<NgZoneZone>global.zone)
Copy link
Contributor

Choose a reason for hiding this comment

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

<Zone>global.zone

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch, done.

@juliemr juliemr mentioned this pull request Nov 19, 2015
Closed
vicb
vicb reviewed Nov 19, 2015
View reviewed changes
modules/angular2/test/testing/testing_public_spec.ts
' returned value was: true');
restoreJasmineBeforeEach();
});
it('should fail when an asyncornous error is thrown', (done) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

typo: asynchronous

@vicb
Copy link
Contributor

vicb commented Nov 19, 2015

A few minor comments, otherwise LGTM.
Great PR !

@juliemr juliemr force-pushed the zones-for-tests branch 2 times, most recently from 53f6396 to d3f814c Compare November 19, 2015 18:58
@IgorMinar
Copy link
Contributor

@wardbell fyi - this will affect testing docs

@IgorMinar
Copy link
Contributor

looks great! Please deprecate injectAsync and add docs about the async behavior for inject

@IgorMinar IgorMinar added action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews pr_state: LGTM and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Nov 19, 2015
@IgorMinar IgorMinar assigned juliemr and unassigned IgorMinar Nov 19, 2015
@IgorMinar
Copy link
Contributor

OMG, I just realized that this might mean that we don't need to refactor all of our tests in angular2/angular2 to return promises. Now I'm really stoked.

Do we properly detect errors and surface them or will we still see jasmine timeouts when an error occurs in an async test?

@juliemr
Copy link
Member Author

juliemr commented Nov 19, 2015

Errors should be properly output: As in this test:

    it('should fail when an asynchronous error is thrown', (done) => {
      var itPromise = patchJasmineIt();

      it('throws an async error',
         inject([], () => { setTimeout(() => { throw new Error('bar'); }, 0); }));

      itPromise.then(() => { done.fail('Expected function to throw, but it did not'); }, (err) => {
        expect(err.message).toEqual('bar'); // <-- bar not just timeout!
        done();
      });
      restoreJasmineIt();
    });

@wardbell
Copy link
Contributor

Big win! Looking forward to trying this.

@juliemr
Copy link
Member Author

juliemr commented Nov 19, 2015

Deprecation notice and docs are done.

@juliemr juliemr added action: merge The PR is ready for merge by the caretaker and removed action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews labels Nov 19, 2015
@juliemr juliemr mentioned this pull request Nov 24, 2015
Closed
@vsavkin
Copy link
Contributor

vsavkin commented Nov 25, 2015

Could you rebase?

@vsavkin vsavkin added action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews and removed action: merge The PR is ready for merge by the caretaker labels Nov 25, 2015
@juliemr
feat(testing): use zones to avoid the need for injectAsync
7d3de24 
Use a zone counting timeouts and microtasks to determine when a test
is finished, instead of requiring the test writer to use
injectAsync and return a promise.

See angular#5322
@juliemr
Copy link
Member Author

juliemr commented Nov 25, 2015

@vsavkin done!

@juliemr juliemr removed the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Nov 25, 2015
@IgorMinar
Copy link
Contributor

let's get this in! :)

@juliemr juliemr assigned vsavkin and unassigned alxhub Nov 25, 2015
@vsavkin vsavkin added action: merge The PR is ready for merge by the caretaker zomg_admin: do merge and removed zomg_admin: do merge labels Nov 30, 2015
@vsavkin
Copy link
Contributor

vsavkin commented Nov 30, 2015

Merged via 0c9596a

@vsavkin vsavkin closed this Nov 30, 2015
@juliemr juliemr mentioned this pull request Nov 30, 2015
Closed
juliemr added a commit to juliemr/angular that referenced this pull request Dec 9, 2015
@juliemr
refactor(testing): reenable injectAsync checking for return value
e76b9ea 
Before angular#5375, injectAsync would check the return value and fail
if it was not a promise, to help users remember that they need to
return a promise from an async test. angular#5375 removed that with the
introduction of the testing zone.

This un-deprecates `injectAsync` until we can resolve
angular#5515.

To be clear, this means that `inject` and `injectAsync` are now
identical except that `injectAsync` will fail if the test
does not return a promise.
@juliemr juliemr mentioned this pull request Dec 9, 2015
Closed
juliemr added a commit to juliemr/angular that referenced this pull request Dec 15, 2015
@juliemr
refactor(testing): reenable injectAsync checking for return value
354f9f8 
Before angular#5375, injectAsync would check the return value and fail
if it was not a promise, to help users remember that they need to
return a promise from an async test. angular#5375 removed that with the
introduction of the testing zone.

This un-deprecates `injectAsync` until we can resolve
angular#5515.

To be clear, this means that `inject` and `injectAsync` are now
identical except that `injectAsync` will fail if the test
does not return a promise, and `inject` will fail if the test
returns any value.
juliemr added a commit to juliemr/angular that referenced this pull request Dec 15, 2015
@juliemr
refactor(testing): reenable injectAsync checking for return value
3018614 
Before angular#5375, injectAsync would check the return value and fail
if it was not a promise, to help users remember that they need to
return a promise from an async test. angular#5375 removed that with the
introduction of the testing zone.

This un-deprecates `injectAsync` until we can resolve
angular#5515.

To be clear, this means that `inject` and `injectAsync` are now
identical except that `injectAsync` will fail if the test
does not return a promise, and `inject` will fail if the test
returns any value.
juliemr added a commit to juliemr/angular that referenced this pull request Dec 15, 2015
@juliemr
refactor(testing): reenable injectAsync checking for return value
963f5b2 
Before angular#5375, injectAsync would check the return value and fail
if it was not a promise, to help users remember that they need to
return a promise from an async test. angular#5375 removed that with the
introduction of the testing zone.

This un-deprecates `injectAsync` until we can resolve
angular#5515.

To be clear, this means that `inject` and `injectAsync` are now
identical except that `injectAsync` will fail if the test
does not return a promise, and `inject` will fail if the test
returns any value.
juliemr added a commit to juliemr/angular that referenced this pull request Dec 15, 2015
@juliemr
refactor(testing): reenable injectAsync checking for return value
1f46b66 
Before angular#5375, injectAsync would check the return value and fail
if it was not a promise, to help users remember that they need to
return a promise from an async test. angular#5375 removed that with the
introduction of the testing zone.

This un-deprecates `injectAsync` until we can resolve
angular#5515.

To be clear, this means that `inject` and `injectAsync` are now
identical except that `injectAsync` will fail if the test
does not return a promise, and `inject` will fail if the test
returns any value.
juliemr added a commit to juliemr/angular that referenced this pull request Dec 15, 2015
@juliemr
refactor(testing): reenable injectAsync checking for return value
4804da5 
Before angular#5375, injectAsync would check the return value and fail
if it was not a promise, to help users remember that they need to
return a promise from an async test. angular#5375 removed that with the
introduction of the testing zone.

This un-deprecates `injectAsync` until we can resolve
angular#5515.

To be clear, this means that `inject` and `injectAsync` are now
identical except that `injectAsync` will fail if the test
does not return a promise, and `inject` will fail if the test
returns any value.
juliemr added a commit that referenced this pull request Dec 15, 2015
@juliemr
refactor(testing): reenable injectAsync checking for return value
b803ecf 
Before #5375, injectAsync would check the return value and fail
if it was not a promise, to help users remember that they need to
return a promise from an async test. #5375 removed that with the
introduction of the testing zone.

This un-deprecates `injectAsync` until we can resolve
#5515.

To be clear, this means that `inject` and `injectAsync` are now
identical except that `injectAsync` will fail if the test
does not return a promise, and `inject` will fail if the test
returns any value.

Closes #5721
@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 7, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees

@vsavkin vsavkin

Labels
action: merge The PR is ready for merge by the caretaker area: testing Issues related to Angular testing features, such as TestBed cla: yes
Projects
None yet
Milestone
beta.0
Development

Successfully merging this pull request may close these issues.
10 participants
@juliemr @0x-r4bbit @vicb @IgorMinar @wardbell @Foxandxss @alxhub @vsavkin @ajoslin @googlebot