Load one ad a time. #9808
Load one ad a time. #9808
Conversation
| win[LOADING_ADS_WIN_ID_]--; | ||
| }, 1000); | ||
| timerFor(win) | ||
| .timeoutPromise(1000, opt_loadingPromise || new Promise(() => {})) |
There was a problem hiding this comment.
Instead of creating a never resolving promise, can't you just pass null?
| expect(is3pThrottled(win)).to.be.false; | ||
| incrementLoadingAds(win); | ||
| expect(is3pThrottled(win)).to.be.true; | ||
| clock.tick(999); | ||
| yield macroTask(); |
There was a problem hiding this comment.
You could return the promise from incrementLoadingAds.
There was a problem hiding this comment.
The test then becomes:
clock.tick(999);
expect(is3pThrottled(win)).to.be.true;
clock.tick(1);
yield promise;
expect(is3pThrottled(win)).to.be.false;the first check is meaningless here.
There was a problem hiding this comment.
it's unfair to only yield on the 2nd tick. (and you can't yield on the first, which will run into a deadlock :-))
this is not testing the right thing:
clock.tick(1000);
expect(is3pThrottled(win)).to.be.true;
clock.tick(1);
yield promise;
expect(is3pThrottled(win)).to.be.false;There was a problem hiding this comment.
Right now, the macroTask isn't testing anything. You've frozen time in test's window.
There was a problem hiding this comment.
I agree we should yield before doing each check.
Here we try to test that loading ads count will decrease by 1 after 1 sec, and only after 1 sec.
With yield promise, we can test that the ads count decrease by 1 after 1 sec.
However with
clock.tick(999);
expect(is3pThrottled(win)).to.be.true;
we can't tell if the promise is resolved only after 1sec. It is possible that the promise has already resolved after 999ms, but macroTasks not executed, loading ad count hasn't decresed by 1 when test code ran.
This is probably right to do:
clock.tick(999);
yield macroTask();
expect(is3pThrottled(win)).to.be.true;
clock.tick(1);
yield promise;
expect(is3pThrottled(win)).to.be.false;
There was a problem hiding this comment.
Right now, the macroTask isn't testing anything. You've frozen time in test's window.
@jridgewell here we want to yield an event loop so that if the timer has triggered the promise resolver, we wait till the resolution finish.
@zhouyx your suggestion is also treating the 2 ticks differently, which I don't like
There was a problem hiding this comment.
I'd agree with @zhouyx here, if you want to test specifically that it only resolves after 1s. I'm still against relying on macrotasks for promise changes to complete, though.
There was a problem hiding this comment.
why do we want to treat the 2 ticks differently? isn't it cheating? are we doing a test to just get it passed?
we have to do 2 checks with all other conditions the same, if we really want to test the 1000ms triggering logic.
the test is translated to:
- clock tick 999ms
- ad should still be throttled in the next event loop (1ms to go!)
- clock tick 1ms
- ad should NOT be throttled starting from the next event loop
| win[LOADING_ADS_WIN_ID_]--; | ||
| }, 1000); | ||
| timerFor(win) | ||
| .timeoutPromise(1000, opt_loadingPromise || new Promise(() => {})) |
There was a problem hiding this comment.
timeoutPromise() only require the timeout value to be defined. would timeoutPromise(1000, opt_loadingPromise) be enough?
Closes #9149