fix($interval): change $interval ngMock service to handle second argu… #15953

luciomartinez · 2017-05-02T02:47:18Z

…ment equivalent to zero

Enable test interval calls with delay set to zero without infinite loop.
If a spec has $interval.flush(ms) and the code to test has
$interval(callback, 0) then instead of running an infinite while loop
it would move forward ms executing the $interval's callback.

Fixes #15952

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
Bug fix

What is the current behavior? (You can also link to an open issue here)
#15952

What is the new behavior (if this is a feature change)?

Does this PR introduce a breaking change?
No.

Please check if the PR fulfills these requirements

The commit message follows our guidelines: https://github.com/angular/angular.js/blob/master/CONTRIBUTING.md#commit-message-format
Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

Other information:

googlebot · 2017-05-02T02:47:21Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for the commit author(s). If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.

googlebot · 2017-05-02T02:47:21Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for the commit author(s). If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.

gkalpak · 2017-05-02T12:10:28Z

src/ngMock/angular-mocks.js

@@ -556,9 +556,13 @@ angular.mock.$IntervalProvider = function() {
     */
    $interval.flush = function(millis) {
      now += millis;
-      while (repeatFns.length && repeatFns[0].nextTime <= now) {
+      var firstTask = repeatFns[0];


gkalpak · 2017-05-02T12:10:36Z

src/ngMock/angular-mocks.js

        var task = repeatFns[0];
        task.fn();
+        if (!task.delay) {
+          task.delay = task.nextTime = 1;
+        }


Wrong indentation.

Sorry, I set two spaces as indentation. Is it wrong or I'm missing something else?

Sorry, it turned out I had a Chrome extension that was messing the GitHub diffs. Indentation is fine.

No worries 👍

gkalpak · 2017-05-02T12:13:35Z

src/ngMock/angular-mocks.js

        var task = repeatFns[0];
        task.fn();
+        if (!task.delay) {
+          task.delay = task.nextTime = 1;


Why set nextTime to 0 (since we are also incrementing it below)?

If delay is zero then nextTime will also be zero. Setting to 1 means the next round it'll be 2 since in fact will be the round number 2. Thanks to first set to 1 and then increasing it means that passing a value of 1 to ms will run only once and not twice the function. I hope this makes sense :-)

I'm wondering if it'd be helpful to write this down as a comment. What do you think? @gkalpak

A short comment wouldn't hurt for sure 😃
(Feel free to ping me when this is ready for another review.)

Hey thanks! I was afraid of bothering you 😄
It should be ready now, please let me know if I'm missing something else.

gkalpak · 2017-05-02T12:14:47Z

test/ngMock/angular-mocksSpec.js

+      expect(counter).toBe(2000);
+      $interval.flush(1000);
+      expect(counter).toBe(4000);
+    }));


We need a test with other, non-zero-delay timeouts, to verify that the relative orders are correct.

There are specs with non-zero-delay timeouts but maybe you mean mixing them. What do you reckon? Thanks

Yes, I meant mixing zero and non-zero intervals in the same test.

gkalpak · 2017-05-02T12:15:41Z

test/ngMock/angular-mocksSpec.js

+      $interval(function() {counter++;}, 0);
+
+      $interval.flush(1000);
+      expect(counter).toBe(2000);


I would expect this to be 1000. Can you explain why it isn't?

When calling interval it checks the second argument to be a number or equivalent to zero, hence undefined gives the same results. Since above two statements are increasing counter on a flush of 1000 both functions are run and counter finish with a value of 2000.

Duh! Both callbacks increment the same counter.
Still it might be worth having two separate counters to be safer.

Good idea! Will do

googlebot · 2017-05-03T00:30:04Z

CLAs look good, thanks!

googlebot · 2017-05-03T00:30:04Z

CLAs look good, thanks!

gkalpak · 2017-05-22T09:26:25Z

src/ngMock/angular-mocks.js

+        // to avoid running the while loop
+        // one more time than the specified
+        if (!task.delay) {
+          task.delay = task.nextTime = 1;


Actually, I don't think this will work if the zero-delay timer was not added from the beginning (i.e. if there was a flush before adding it). E.g.:

var counter = 0; $interval.flush(1000); $interval(function() { counter++; }); $interval.flush(1000); expect(counter).toBe(1000); // <-- Will probably be 2000

Also, due to how repeatFns.nextTime is set up initially (nextTime: now + delay), leaving the delay undefined would behave differently than using 0 (which is not desirable).

I think we should change both $interval and $interval.flush to something like:

$interval = function(fn, delay, ...) { ... repeatFns.push({ nextTime: (now + (delay || 0)), delay: delay || 1, ... });

$interval.flush = function(millis) { var before = now; now += millis; while (repeatFns.length && repeatFns[0].nextTime <= now) { ... if (task.nextTime === before) { // This can only happen the first time a zero-delay interval gets triggered. task.nextTime++; } task.nextTime += task.delay; ...

Then we need to have more tests that verify the correct behavior of zero-delay intervals (with both delay: 0 and delay: undefined) for at least the following cases:

Interval being added when now > 0.

Calling flush(0) (should only trigger zero-delay intervals and only trigger them once).

Calling flush(0) for a second time (should not trigger any intervals).

Nice! I wasn't sure about changing $interval but I like this implementation. Thanks!

gkalpak · 2017-05-26T10:00:59Z

src/ngMock/angular-mocks.js

      now += millis;
      while (repeatFns.length && repeatFns[0].nextTime <= now) {
        var task = repeatFns[0];
        task.fn();
+        if (task.nextTime === before) {
+          // this can only happen the first time
+          // a zero-delay interval get triggered


get --> gets

gkalpak · 2017-05-26T10:02:52Z

test/ngMock/angular-mocksSpec.js

+
+      expect(counterA).toBe(0);
+      expect(counterB).toBe(0);
+    }));


We need another $interval.flush(X) and assertions that the callbacks where run only X times.

…ment equivalent to zero Enable test interval calls with delay set to zero without infinite loop. If a spec has `$interval.flush(ms)` and the code to test has `$interval(callback, 0)` then instead of running an infinite while loop it would move forward `ms` executing the $interval's callback. Fixes #15952

luciomartinez · 2017-06-03T20:51:52Z

@gkalpak updated changes. Thanks!

Previously, trying to test code thaat contained zero-delay intervals (e.g. `$interval(fn, 0)` or `$interval(fn)`) would result in an infinite loop. This commit avoids the infinite loop, by treating zero-delay intervals as one second intervals (except for the initial trigger, where they can also be executed with `$interval.flush(0)`). Fixes #15952 Closes #15953

googlebot added the cla: no label May 2, 2017

gkalpak suggested changes May 2, 2017

View reviewed changes

gkalpak added component: ngMock needs: test needs: work severity: crash / freeze type: feature labels May 2, 2017

gkalpak added this to the Backlog milestone May 2, 2017

googlebot added cla: yes and removed cla: no labels May 3, 2017

gkalpak suggested changes May 22, 2017

View reviewed changes

gkalpak suggested changes May 26, 2017

View reviewed changes

gkalpak approved these changes Jun 5, 2017

View reviewed changes

gkalpak removed needs: test needs: work labels Jun 5, 2017

gkalpak modified the milestones: 1.6.5, Backlog Jun 5, 2017

gkalpak closed this in 32f4645 Jun 6, 2017

fix($interval): change $interval ngMock service to handle second argu… #15953

fix($interval): change $interval ngMock service to handle second argu… #15953

Uh oh!

Conversation

luciomartinez commented May 2, 2017

Uh oh!

googlebot commented May 2, 2017

Uh oh!

googlebot commented May 2, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

luciomartinez May 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

googlebot commented May 3, 2017

Uh oh!

googlebot commented May 3, 2017

Uh oh!

gkalpak May 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gkalpak May 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

luciomartinez commented Jun 3, 2017

Uh oh!

Uh oh!

luciomartinez May 4, 2017 •

edited

Loading

gkalpak May 22, 2017 •

edited

Loading

gkalpak May 26, 2017 •

edited

Loading