Remove timer calls to prevent future flakiness #9478

cvializ · 2017-05-22T22:17:14Z

No issue tracks this, I'm just trying to find a good way to simplify asynchronous tests without using timer. Hopefully if this is a good approach we can replace other usages of timer outside of amp-form's tests.

stubWithCalledPromise looks kind of weird because sandbox.stub returns a stub instance when given two arguments, but strangely returns a spy instance when passed a third callback argument. This prevents us from using the stub api e.g. stub.returns(Promise.resolve()) to adjust the behavior of the stub. stubWithCalledPromise allows us to use the three argument invocation to resolve the promise while still returning a stub instance.

dvoytenko · 2017-05-23T21:25:22Z

Thanks @cvializ ! On my quick scan it looks good, but I'll defer to @jridgewell and @aghassemi

aghassemi · 2017-05-24T00:12:40Z

extensions/amp-form/0.1/test/test-amp-form.js

@@ -330,7 +331,9 @@ describes.repeated('', {
        emailInput.setAttribute('required', '');
        form.appendChild(emailInput);
        sandbox.spy(form, 'checkValidity');
-        sandbox.stub(ampForm.xhr_, 'fetch').returns(Promise.resolve());
+        const {promise, stub} = stubWithCalledPromise(


Like the idea, have a suggestion for an alternative API:

var stub = sandbox.stub(ampForm.xhr_, 'fetch').returns(Promise.resolve()); ..... ..... whenCalled(stub).then()....

also optionally:

// when called for the third time. whenCalled(stub, 3).then()....

on the implementation side of whenCalled, you can use poll method we have in iframe.js (probably can move somewhere else) to check for callCount (poll already returns a promise). This should work for both stub and spy since they both have callCount.

WDYT?

That looks good, but wouldn't polling would reintroduce flakiness? This implementation definitively resolves the promise and allows the test to continue as soon as the stubbed method is called.

polling on callCount shouldn't cause flakiness only impact would be promise getting resolved up to 8ms later than the call time. poll also takes an optional timeout (defaults to 1600ms) which means in the case of test failure (waiting for a stub call that never happens) it will timeout independent of the test timing out which provides more information on where test failure happened.

jridgewell · 2017-05-24T00:34:58Z

testing/test-helper.js

+        original.apply(object, spy.lastCall.args);
+      }
+
+      resolve({value: result});


Why not just resolve with result?

If result is a promise, passing it to resolve will continue the promise chain. The test will hang if a test stubs a method and changes the stub behavior to return a promise, and the test doesn't immediately resolve the promise. I'm open to alternative solutions. See https://github.com/ampproject/amphtml/pull/9478/files#diff-f6081f6237f5cc3def355c9a96a881cdR718

jridgewell · 2017-05-24T00:36:08Z

testing/test-helper.js

+    const spy = sandbox.stub(object, method, () => {
+      const result = stub.apply(object, spy.lastCall.args);
+      if (opt_callMethod) {
+        original.apply(object, spy.lastCall.args);


You can avoid this weirdness by switching between stub and spy:

const spy = sandbox[opt_callMethod ? 'spy' : 'stub'](object, method, () => { });

The Spy constructor doesn't accept a third argument.

jridgewell · 2017-05-24T00:36:28Z

testing/test-helper.js

+  const promise = new Promise(resolve => {
+    stub = sandbox.stub();
+    const spy = sandbox.stub(object, method, () => {
+      const result = stub.apply(object, spy.lastCall.args);


Why are we using a stub and a spy together?

The short answer is because the Sinon stub API is weird.

The long answer is Sinon doesn't implement a promise API for when a stub is called, so I'm implementing that here. The stub constructor does accept a callback argument, which I use to resolve the promise, but it returns a spy instance instead of a stub instance. If a test needs to change a stub's behavior, e.g. calling stub.returns(...), it has no reference to the stub. Even object.method is a spy if you pass a callback argument to sandbox.stub.

We could avoid using a spy and a stub together using Stub#callsFake in Sinon 2.0, but we are using 1.17

jridgewell · 2017-05-25T21:01:25Z

testing/test-helper.js

+  const promise = new Promise(resolve => {
+    stub = sandbox.stub();
+    const spy = sandbox.stub(object, method, () => {
+      const result = stub.apply(object, spy.lastCall.args);


object here could be the incorrect context. I think spy.lastCall has a context property?

cvializ · 2017-06-01T22:50:16Z

PTAL when you get a chance. I switched to Ali's suggested whenCalled API

aghassemi

sweet

aghassemi · 2017-06-01T23:32:42Z

testing/test-helper.js

@@ -44,6 +45,11 @@ export function stubServiceForDoc(sandbox, ampdoc, serviceId, method) {
  return sandbox.stub(service, method);
 }

+export function whenCalled(stub, opt_callCount = 1) {


aghassemi · 2017-06-01T23:37:25Z

testing/test-helper.js

@@ -44,6 +45,11 @@ export function stubServiceForDoc(sandbox, ampdoc, serviceId, method) {
  return sandbox.stub(service, method);
 }

+export function whenCalled(stub, opt_callCount = 1) {


technically it is whenCalled(spy, ...) (Sinon's Stub implements Spy)

aghassemi · 2017-06-01T23:45:30Z

extensions/amp-form/0.1/test/test-amp-form.js

-        return timer.promise(10).then(() => {
-          expect(ampForm.xhr_.fetch).to.have.been.called;
+
+        return whenCalled(ampForm.xhr_.fetch).then(() => {


the then part is a no-op in this case (I'd still keep it to be explicit however)

cool, that was my thinking too.

Feedback addressed, ready to merge

* Remove timer calls to prevent future flakiness * Revert the line order change * Poll instead of using stub callback constructor * Add jsdoc and rename variables

* Bidtellect amp implementation… (ampproject#9518) * Bidtellect amp implementation… * removing renderStartImplemented… * spaces? * adding example * Fix test-amp-ad-network-doubleclick-impl (ampproject#9645) * Optimizations to speed up Travis PR builds (ampproject#9626) Travis builds have become slow during busy periods of the day due to various reasons. This PR does the following: Reorganizes build tasks across fewer build shards Reduces the number of VMs used by PR and master builds to 2 No longer does a full gulp dist for unit tests (Possible to do after ampproject#9404) Reduces the total CPU time used for PR builds to < 20 mins Fixes ampproject#9500 Fixes ampproject#9387 * Animations: full on-action API (ampproject#9641) * Animations: full on-action API * review fixes * Fix Validator tests on Travis, which now uses Node v4 (ampproject#9654) * Amp-imgur : Implement imgur embed (ampproject#9405) (ampproject#9434) * Amp-imgur : Implement imgur embed (ampproject#9405) * amp-imgur test code modified * amp-imgur validation code changed * fix amp-imgur lint check * Remove unused Layour variables * Test code modified * Ingegration amp-imgur extension * [amp-imgur] add resize logic * Remove 3p iframes and add resize message listener * Fix some code with reviews * change some syntax * change validation rules * add find event source * [amp-imgur] remove unnecessary code and add object check * remove spec_name from validation code * Remove unnecessary code and add owners file * Change required tag name to in validation file * data-imgurid -> data-imgur-id * Add whitelist for amp-imgur * remove unused AmpImgur import in test file * remove unused preconnect callback * Update amp-cors-requests.md (ampproject#9636) Added updates and corrections per Dima's feedback. * Expose login url building to amp access service (ampproject#9300) * Animations: support style keyframes (ampproject#9647) * Animations: support style keyframes * docs * review fixes * lints * Animations: subtargets format (ampproject#9655) * Animations: subtargets format * fix docs * Fix extern for integration test (ampproject#9657) * Remove whitelisted link for PR 9434 (ampproject#9656) * Add callouts for CORS + cleanup (ampproject#9591) * Add callouts for CORS + cleanup * Update amp-access.md * Doubleclick Fast Fetch: Send default safeframe version on ad request (ampproject#9613) * Send default safeframe version on ad request * fix test failure * amp-ima-video: Fixes some undefined variables in compiled extension (ampproject#9617) * Add experimental input-debounced to the actions and event doc (ampproject#9650) * Copy and rename compiled script directly for deprecated version (ampproject#9587) * Remove A4A Dependency on ads/_config.js (ampproject#9462) * Added doubleclick specific config * Modified other networks * Update MockA4AImpl * Remove unneeded config lines * Add cid, remove renderStartImplemented * Fix lint complaint * Changed from mandatory config to overrrideable method * Addressed feedback, modified getAdCid * Addressed additional feedback * Responded to feedback * Removed unnneeded guard against null value * Changed signature of function * Fixed lint issues * addressed comments * Addressed comments, fixed broken test * Change signature / update tests * Fix wrong contents and white spaces in amp-imgur.md (ampproject#9658) * Performance: separate ini-load from first visible ini-load (ampproject#9665) * amp-bind: Fix more integration tests (ampproject#9598) * more bind test fixes * include object.assign polyfill * move polyfills to separate file * replace use of map() from bind-validator with ownProperty() * fix types and test * Stop using IOS elastic scroll when slidescroll takes over animation (Only for the NON SNAP-POINTS flow) (ampproject#9668) * removing overflow touch on no-scroll for ios * Stop using IOS elastic scroll when slidescroll takes over animation (Only for the NON SNAP-POINTS flow) * make sure css is compiled before entry points (ampproject#9643) * update npm package version (ampproject#9671) * A4A: expose visibilityCsi (ampproject#9667) * A4A: expose visibilityCsi * tests * amp-bind: Fix embedding in FIF (ampproject#9541) * move element service changes from ampproject#9447 * store element service ids in extension struct * add unit test, fix other tests * Fix null value binding (ampproject#9674) * Add amp-form's submit action to the docs (ampproject#9675) * Write cookie for ad-cid. (ampproject#9594) * Write cookie for ad-cid. * fix tests * amp-access-laterpay fixes (ampproject#9633) * Add support for LaterPay subscriptions * Fix canonical link on laterpay example * Tweak default styling * Add an example locale message for the header * Fix indentation of JsDoc (ampproject#9677) It should be indented at the same level as the method it's documenting. * Add example to amp iframe docs (ampproject#9660) * Change image to static link hosted on ampproject.org * Update amp-iframe doc w example + gen cleanup * Adds SFG experiment branches to doubleclick-a4a-config.js. (ampproject#9662) * Added experiment branches corresponding to exp=a4a:(5|6). * Removed test file. * Removed changes to yarn.lock + minor fixes. * Fixed adsense-a4a-config.js. * Fixed return statement. * Removed reference to style tag for opacity (ampproject#9634) As style tag for `opacity` was replaced with `amp-boilerplate (back in this [commit](ampproject@0a056ca), updated the text to reflect those changes. * De-flake amp-bind integration tests (ampproject#9683) * split bind fixtures into one file per extension * fix width/height binding bug, avoid race condition w/ dynamic containers * actually, just skip the tests affected by race condition for now * Remove timer calls to prevent future flakiness (ampproject#9478) * Remove timer calls to prevent future flakiness * Revert the line order change * Poll instead of using stub callback constructor * Add jsdoc and rename variables * amp-animation: polyfill partial keyframes from CSS (ampproject#9689) * Animations: whitelist offset distance and regroup condition types (ampproject#9688) * Use Docker containers in Travis (ampproject#9666) * clean up web-worker experiment (ampproject#9706) * Load examiner.js when #development=2 (ampproject#9680) * Load examiner.js when #development=1 * address comments * Use development=2 * Fix presubmit * Implementation of amp-ad-exit (ampproject#9390) * Implementation of the amp-ad-exit component for managing ad navigation (ampproject#8515). Changes from the I2I: - Only RANDOM, CLICK_X, and CLICK_Y variables can be used, alongside custom vars. - It doesn't work well with <a> tags - the tap action doesn't trigger on middle clicks. Recommendation is to use other elements. - ClickLocationFilter for border click protection is not yet implemented. It will be added in a future change. The component does some lightweight validation of the JSON config when it's built. This may be overkill. Tested: gulp test --files 'extensions/amp-ad-exit/0.1/test/*' gulp check-types * Update JSDoc for some functions. Fix lint issue (ignore unused parameters in an interface method). * sinon.spy -> sandbox.spy * Add amp-ad-exit to the list of allowed extensions in A4A pages and fix validation rules. Address PR: add experiment flag; use camelCase for config properties address some comments Instantiate a new Filter for each spec. * Instantiate a new Filter for each spec. * remove duplicate doctype tag in example * Make ActionEventDef public for type annotations. * Add filter factory. * fix event var * update type annotation for Filter.filter * address PR comments * Move filter config validation to the ctor. Remove Filter.buildCallback(). * camelCase in documentation examples * Introducing "it.yield", a convenient way to test promise code (ampproject#9601) * Introduce yield. * Address comments. * fix done state error handling * Document amp-access as a special target (ampproject#9568) `amp-access` is a special target that's not mentioned in this document. It's special because you can't give an arbitrary ID to it, i.e., you can't just change the id of the `amp-access` script tag to `amp-access2` and expect `on="tap:amp-access2.login"` to work. Given that the "actions" for `amp-access` is a bit dynamic depending on the structure of the `amp-access` JSON, instead of listing out the actions in a tabular form, a reference to the `amp-access` documentation is added. * Run presubmit tests soon after building (ampproject#9716) * Revert "Use Docker containers in Travis (ampproject#9666)" (ampproject#9717) This reverts commit 98ecb9b. It turns out that while using Docker containers instead of VMs on Travis reduces VM startup time, it has significantly increased build and test time, increasing the overall PR check round trip from < 20 mins to > 30 mins. Compare https://travis-ci.org/ampproject/amphtml/jobs/238464404 (Docker) against https://travis-ci.org/ampproject/amphtml/jobs/238462066 (VM). Fixes ampproject#9651 * Validator Rollup (ampproject#9719) * Add validator tests for amp-imgur. * Disallow amp-embed as child of amp-app-banner. * Minor cleanup, Make sure our set numbers are consistent in javascript. * amp-state: Allow both `src` and script child (ampproject#9721) * adapt code from ampproject#8897 * also_requires_attr should be in trigger * Remove experiment for input-debounced. Update docs. (ampproject#9724) * Validator Rollup (ampproject#9727) * Minor cleanup of amp-ad-exit * Disallow amp-ad/amp-embed as children of amp ad containers when data-multi-size is present. * Revision bump * Enabling dynamic queryparam addition to anchour links (ampproject#9684) * Enabling queryparam addition to anchour links * Code review changes * Additional cache urls (ampproject#9733) * Significantly speed up gulp build --css-only (ampproject#9726) Prior to this PR, gulp build --css-only would take ~ 1 minute to run, and end up building a bunch of js files in addition to compiling css. After this PR, gulp build --css-only takes ~ 0.5 seconds to run (a 100x speed up), and the only output files in build/ after running it are .css, and .css.js files. Resulting speeds on my workstation: gulp build --css-only: ~500ms gulp build: ~1m 45s gulp dist --fortesting: ~3m 30s Fixes ampproject#9640 * add amp-analytics Facebook Pixel support (ampproject#9449) * add amp-analytics Facebook Pixel support * add amp-analytics Facebook Pixel support * fix test on extensions/amp-analytics/0.1/test/vendor-requests.json * add Facebook Pixel standard events https://developers.facebook.com/docs/ads-for-websites/pixel-events/v2.9#events * Update vendors.js fix conflicts * Update vendors.js fix conflicts * Update vendor-requests.json fix conflicts * Update vendor-requests.json fix conflicts * Update vendor-requests.json * Update analytics.amp.html * Update analytics-vendors.amp.html * Update vendors.js * Update analytics-vendors.amp.html * fix lint error * update * facebook pixel * Update vendor-requests.json * Update analytics.amp.html * fix lint error * fix lint error * fix alphabetical order * Update yarn.lock * AMP-ad supports Seznam Imedia ad network (ampproject#8893) * AMP-ad supports Seznam Imedia ad network * Fixed requested changes * Fixed requested changes * Render API implemented * Improved placing ads * Fixed requested changes * Render API fixed context * Renamed IM.cz to Imedia * Described JSON parameters

cvializ requested review from jridgewell, dvoytenko and aghassemi May 22, 2017 22:17

googlebot added the cla: yes label May 22, 2017

cvializ force-pushed the cv-timer branch 2 times, most recently from 480499d to db56892 Compare May 23, 2017 17:02

dvoytenko removed their request for review May 23, 2017 21:24

cvializ force-pushed the cv-timer branch from db56892 to 1cf4104 Compare May 23, 2017 21:59

aghassemi reviewed May 24, 2017

View reviewed changes

jridgewell previously requested changes May 24, 2017

View reviewed changes

jridgewell reviewed May 25, 2017

View reviewed changes

Carlos Vializ added 2 commits May 31, 2017 16:09

Remove timer calls to prevent future flakiness

657e9b6

Revert the line order change

adffa95

cvializ force-pushed the cv-timer branch from 356e035 to adffa95 Compare June 1, 2017 15:42

Poll instead of using stub callback constructor

36823cb

aghassemi approved these changes Jun 1, 2017

View reviewed changes

Add jsdoc and rename variables

5599db3

cvializ merged commit a398bb0 into ampproject:master Jun 2, 2017

cvializ deleted the cv-timer branch June 2, 2017 17:28

cvializ mentioned this pull request Jun 5, 2017

Weekly Status 2017-06-05 #9714

Closed

ampprojectbot added the PR use: In Beta / Experimental label Jun 12, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove timer calls to prevent future flakiness #9478

Remove timer calls to prevent future flakiness #9478

cvializ commented May 22, 2017

dvoytenko commented May 23, 2017

aghassemi May 24, 2017

cvializ May 24, 2017

aghassemi May 24, 2017

jridgewell May 24, 2017

cvializ May 24, 2017

jridgewell May 24, 2017

cvializ May 24, 2017

jridgewell May 24, 2017

cvializ May 24, 2017

jridgewell May 25, 2017

cvializ commented Jun 1, 2017

aghassemi left a comment

aghassemi Jun 1, 2017

aghassemi Jun 1, 2017

aghassemi Jun 1, 2017

cvializ Jun 2, 2017

Remove timer calls to prevent future flakiness #9478

Remove timer calls to prevent future flakiness #9478

Conversation

cvializ commented May 22, 2017

dvoytenko commented May 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cvializ commented Jun 1, 2017

aghassemi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment