Skip to content
This repository was archived by the owner on Apr 12, 2024. It is now read-only.

fix(ngMock): call $interval callbacks even when invokeApply is false #10032

Closed
wants to merge 1 commit into from
Closed

fix(ngMock): call $interval callbacks even when invokeApply is false #10032

wants to merge 1 commit into from

Conversation

terite
Copy link
Contributor

@terite terite commented Nov 13, 2014

Functions given to ngMock's $interval aren't being called after $interval.flush(...) when invokeApply is false. This is because $q.notify does work inside nextTick, and the nextTick provided to $q (which is $rootScope.$evalAsync), doesn't tick until a digest.

I mostly took changes from 19b6b34

I'm not really happy with the fix, but I don't know if it's actually a problem. It would be nice if ngMock's $interval was a decorator like $timeout, but I think that's too much of a change for my first PR.

edit: forgot a word, I'm not happy with the fix

@mary-poppins
Copy link

I'm sorry, but I wasn't able to verify your Contributor License Agreement (CLA) signature. CLA signature is required for any code contributions to AngularJS.

Please sign our CLA and ensure that the CLA signature email address and the email address in this PR's commits match.

If you signed the CLA as a corporation, please let us know the company's name.

Thanks a bunch!

PS: If you signed the CLA in the past then most likely the email addresses don't match. Please sign the CLA again or update the email address in the commit of this PR.
PS2: If you are a Googler, please sign the CLA as well to simplify the CLA verification process.

@terite
Copy link
Contributor Author

terite commented Nov 13, 2014

I signed the CLA, and my email there matches email on the author of this commit.

For the test failure, is this a flaky test? https://travis-ci.org/angular/angular.js/jobs/40842010#L3910

@caitp
Copy link
Contributor

caitp commented Nov 13, 2014

I would really prefer if the defer flushing were explicit within tests, but I guess I don't care that much. Being explicit is a better way of ensuring correctness, you typically don't want these things to be doing stuff you aren't expecting.

That said, it was already digesting on its own, so maybe it doesn't matter that much. @petebacondarwin grant us your thoughts on this

@terite
Copy link
Contributor Author

terite commented Nov 13, 2014

I like how ngMock's $timeout is a small decorator around ng's $timeout.

It would be nice if ngMock's $interval took the same approach, but because ng's $interval relies on $window.setInterval and cancelInterval, they would have to be mocked out. Maybe ngMock should provide $window.setInterval?

@caitp
Copy link
Contributor

caitp commented Nov 14, 2014

I guess this is good since it better matches the behaviour of real $interval, so it's okay I guess. Waiting to have the CLA verified and I think we can land it.

@caitp caitp closed this in d81ff88 Nov 14, 2014
@terite terite deleted the fix-mock-interval branch November 14, 2014 20:45
@diwu1989
Copy link

good job @terite

@caitp
Copy link
Contributor

caitp commented Nov 14, 2014

👏 🍰 🎈

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants