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

fix(ngMock): make $timeout.flushNext not require an expectedDelay as per documentation. #3628

Closed
wants to merge 3 commits into from

Conversation

jelbourn
Copy link
Member

No description provided.

@mary-poppins
Copy link

Thanks for the PR!

  • Contributor signed CLA now or in the past
    • If you just signed, leave a comment here with your real name
  • PR's commit messages follow the commit message format

If you need to make changes to your pull request, you can update the commit with git commit --amend.
Then, update the pull request with git push -f.

Thanks again for your help!

@vojtajina
Copy link
Contributor

I actually think this whole flushNext is wrong. At least It should not do expect (this expect syntax is Jasmine specific and angular-mocks is designed to work with Mocha too).

@matsko @IgorMinar why do we have this flushNext() ? 462ed03 comment does not persuade me ;-) I don't think it's that useful to assert that something was scheduled to happen at exact time delay. I think the original behavior of flush() where you pass an optional argument and all timeouts with less or equal delays are executed (they should also be executed, in a sorted order; sorted by the delay).

@matsko
Copy link
Contributor

matsko commented Aug 17, 2013

@vojtajina the problem with flush(TIME) is that it will always run past all the timeout functions that have a <= TIME value. So if you have the following:

$timeout(fn1, 100);
$timeout(fn2, 0);

$timeout.flush(100);

Then it will run both (since the 2nd one's delay == the flush value). The reason why we put this together is because in $animate you have a $timeout(1) at first and then a $timeout(delay || 0) and when I use flush(1) then I can't get to capture the data between both $timeout calls if there is no delay for the 2nd value (this happens when no CSS animation properties are captured).

@btford
Copy link
Contributor

btford commented Aug 21, 2013

Convert expect to throw.

@matsko: we should consider removing the argument altogether and instead use spies on $timeout in tests where needed.

@ghost ghost assigned IgorMinar Aug 22, 2013
IgorMinar pushed a commit to IgorMinar/angular.js that referenced this pull request Aug 22, 2013
@vojtajina
Copy link
Contributor

I suggest removing flushNext. For most of the use cases, you want just control the time - having "wind" or "flush" method then accepts optional time delay and executes all the callbacks registered with smaller delay (in sorted order) is sufficient.

@IgorMinar I'm taking care of this, unless you stop me ;-)


@matsko If you need something special, spy on the setTimeout and do whatever you want.

For instance, I don't understand why you wanna fire the 100ms timeout first and then the 0ms. AFAIK, this will never happen in real scenario.

@matsko
Copy link
Contributor

matsko commented Aug 29, 2013

@vojtajina I'm doing a refactor on the animation tests to remove flushNext(). The reason why it was useful was because the animation code relied on multiple $timeout calls. Now there is only one that is mandatory. Once this is ready then lets remove it.

@vojtajina
Copy link
Contributor

@jelbourn thanks for your work on this PR. I'm closing it, we are going to remove the flushNext instead.

@vojtajina vojtajina closed this Sep 5, 2013
@vojtajina
Copy link
Contributor

#3895

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.

6 participants