🏗 ✅ Isolate tests by eliminating the use of the global `sinon` sandbox #25587

rsimha · 2019-11-14T17:26:44Z

The majority of AMP's tests reuse a single global sandbox instance provided by the sinon package. From looking at test code, there's a fair amount of inconsistency in how different tests create (sometimes), use, and clean up (not always) their sandbox.

This PR picks up where #23726 left off, and does the following:

Upgrades sinon to v7.5.0
Makes it a lint error to use the globals sinon or sandbox in an AMP test
Fixes O(thousand) tests that currently reuse the global sandbox.
- For legacy describe tests:
  - Resets window.sandbox to a new instance in the top-level beforeEach()
  - Cleans up window.sandbox in the top-level afterEach()
  - Modifies all tests to use window.sandbox instead of the global sinon object
- For newer describes tests:
  - env.sandbox is already reset to a new instance in the setup() method of SandboxFixture
  - Modifies all tests to use env.sandbox instead of the global sinon object
Skips a few tests on Safari because atob and btoa cannot be found (Multiple runtime tests fail on Safari (Sauce Labs) because atob and btoa can't be found #25621)

Fixes #23842
Fixes #25371
Closes #23726
Closes #25588
Closes #25455
Related to #14360
Related to #23837
Related to #25621

amp-owners-bot · 2019-11-14T22:21:38Z

Hey @gmajoulet, these files were changed:

extensions/amp-story/0.1/test/test-amp-story-consent.js
extensions/amp-story/0.1/test/test-amp-story-hint.js
extensions/amp-story/0.1/test/test-amp-story-info-dialog.js
extensions/amp-story/0.1/test/test-amp-story-request-service.js
extensions/amp-story/0.1/test/test-amp-story-share-menu.js
extensions/amp-story/0.1/test/test-amp-story-store-service.js
extensions/amp-story/0.1/test/test-amp-story-system-layer.js
extensions/amp-story/0.1/test/test-amp-story.js
extensions/amp-story/0.1/test/test-analytics.js
extensions/amp-story/0.1/test/test-media-pool.js
extensions/amp-story/0.1/test/test-media-tasks.js
extensions/amp-story/0.1/test/test-navigation-state.js
extensions/amp-story/1.0/test/test-amp-story-bookend.js
extensions/amp-story/1.0/test/test-amp-story-consent.js
extensions/amp-story/1.0/test/test-amp-story-embedded-component.js
extensions/amp-story/1.0/test/test-amp-story-hint.js
extensions/amp-story/1.0/test/test-amp-story-info-dialog.js
extensions/amp-story/1.0/test/test-amp-story-media-query-service.js
extensions/amp-story/1.0/test/test-amp-story-page.js
extensions/amp-story/1.0/test/test-amp-story-request-service.js
extensions/amp-story/1.0/test/test-amp-story-share-menu.js
extensions/amp-story/1.0/test/test-amp-story-store-service.js
extensions/amp-story/1.0/test/test-amp-story-system-layer.js
extensions/amp-story/1.0/test/test-amp-story.js
extensions/amp-story/1.0/test/test-analytics.js
extensions/amp-story/1.0/test/test-full-bleed-animations.js
extensions/amp-story/1.0/test/test-live-story-manager.js
extensions/amp-story/1.0/test/test-media-performance-metrics-service.js
extensions/amp-story/1.0/test/test-media-pool.js
extensions/amp-story/1.0/test/test-media-tasks.js

Hey @newmuis, these files were changed:

extensions/amp-story/0.1/test/test-amp-story-consent.js
extensions/amp-story/0.1/test/test-amp-story-hint.js
extensions/amp-story/0.1/test/test-amp-story-info-dialog.js
extensions/amp-story/0.1/test/test-amp-story-request-service.js
extensions/amp-story/0.1/test/test-amp-story-share-menu.js
extensions/amp-story/0.1/test/test-amp-story-store-service.js
extensions/amp-story/0.1/test/test-amp-story-system-layer.js
extensions/amp-story/0.1/test/test-amp-story.js
extensions/amp-story/0.1/test/test-analytics.js
extensions/amp-story/0.1/test/test-media-pool.js
extensions/amp-story/0.1/test/test-media-tasks.js
extensions/amp-story/0.1/test/test-navigation-state.js
extensions/amp-story/1.0/test/test-amp-story-bookend.js
extensions/amp-story/1.0/test/test-amp-story-consent.js
extensions/amp-story/1.0/test/test-amp-story-embedded-component.js
extensions/amp-story/1.0/test/test-amp-story-hint.js
extensions/amp-story/1.0/test/test-amp-story-info-dialog.js
extensions/amp-story/1.0/test/test-amp-story-media-query-service.js
extensions/amp-story/1.0/test/test-amp-story-page.js
extensions/amp-story/1.0/test/test-amp-story-request-service.js
extensions/amp-story/1.0/test/test-amp-story-share-menu.js
extensions/amp-story/1.0/test/test-amp-story-store-service.js
extensions/amp-story/1.0/test/test-amp-story-system-layer.js
extensions/amp-story/1.0/test/test-amp-story.js
extensions/amp-story/1.0/test/test-analytics.js
extensions/amp-story/1.0/test/test-full-bleed-animations.js
extensions/amp-story/1.0/test/test-live-story-manager.js
extensions/amp-story/1.0/test/test-media-performance-metrics-service.js
extensions/amp-story/1.0/test/test-media-pool.js
extensions/amp-story/1.0/test/test-media-tasks.js

rsimha · 2019-11-15T22:09:11Z

@lannka @jridgewell @choumx At long last, I've managed to get all 10,000+ unit tests to pass on Linux with an upgraded sinon package and zero use of the global sandbox.

While I go through the rest of the test runs to make sure nothing else is broken, can I get you folks to review this? (I'll rebase before merging to resolve any new conflicts.)

rsimha · 2019-11-15T23:33:00Z

Friday EoD update: The only remaining failures have to do with atob and btoa not being found on Safari on Sauce Labs. I've logged #25621 for this, and got a rough idea of the root cause from @jridgewell. If solving this ends up being outside the scope of this PR, I'll punt and skip the handful of tests on Safari.

/cc @ampproject/wg-runtime

lannka

Wow, that's a big change. Thanks for handling this. Looked at the description, I think we're on the right route!

rsimha · 2019-11-19T20:35:59Z

Still blocked by the atob / btoa problem (#25621). Changing atob to self.atob did not work. I've skipped all tests that fail due to this issue on Safari and attempt a separate fix.

rsimha · 2019-11-20T00:14:21Z

@jridgewell I've managed to get this PR to pass all local and remote tests (modulo the atob / btoa mystery for which I've filed #25621).

Can you give this a once over and let me know if it's good to merge?

…oject#25587)

rsimha added Related to: Flaky Tests WG: infra labels Nov 14, 2019

rsimha self-assigned this Nov 14, 2019

amp-owners-bot bot requested a review from dvoytenko November 14, 2019 17:26

googlebot added the cla: yes label Nov 14, 2019

This was referenced Nov 14, 2019

📦 Update dependency sinon to v7.5.0 #23726

Closed

Fix all tests in extensions/amp-a4a/0.1/test/test-amp-a4a.js that rely on a global sinon sandbox #25548

Closed

rsimha removed the request for review from dvoytenko November 14, 2019 17:42

ampproject deleted a comment from amp-owners-bot bot Nov 14, 2019

amp-bundle-size bot requested a review from dreamofabear November 15, 2019 20:15

rsimha mentioned this pull request Nov 15, 2019

🏗✅ Import sinon in _init_tests.js #25455

Closed

rsimha requested review from lannka and jridgewell November 15, 2019 22:06

rsimha marked this pull request as ready for review November 15, 2019 22:09

rsimha mentioned this pull request Nov 15, 2019

Multiple runtime tests fail on Safari (Sauce Labs) because atob and btoa can't be found #25621

Closed

lannka approved these changes Nov 19, 2019

View reviewed changes

rsimha added 6 commits November 20, 2019 11:58

📦 Update dependency sinon to v7.5.0

770859d

Import sinon in _init_tests.js and replace global sandbox

2e6faa1

Make it a lint error to use the global sinon object in tests

3bfdb98

Import sinon in describes.js

1ffdb2f

Eliminate the use of the global sinon object in tests

cfa41ea

Skip tests that rely on atob / btoa during Sauce Labs Safari runs

d540955

jridgewell approved these changes Nov 20, 2019

View reviewed changes

rsimha merged commit 6b2c20d into ampproject:master Nov 20, 2019

rsimha deleted the 2019-11-14-GlobalSinon branch November 20, 2019 19:31

rsimha mentioned this pull request Nov 20, 2019

🐛Fix merge conflict in test-amp-story-embedded-component.js #25698

Merged

micajuine-ho pushed a commit to micajuine-ho/amphtml that referenced this pull request Dec 27, 2019

🏗 ✅ Isolate tests by eliminating the use of the global `sinon… (amppr…

01bd2c5

…oject#25587)

rsimha mentioned this pull request Apr 30, 2020

init-tests.js should not change global DOM #23837

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🏗 ✅ Isolate tests by eliminating the use of the global `sinon` sandbox #25587

🏗 ✅ Isolate tests by eliminating the use of the global `sinon` sandbox #25587

rsimha commented Nov 14, 2019 •

edited

amp-owners-bot bot commented Nov 14, 2019

rsimha commented Nov 15, 2019 •

edited

rsimha commented Nov 15, 2019

lannka left a comment

rsimha commented Nov 19, 2019 •

edited

rsimha commented Nov 20, 2019 •

edited

🏗 ✅ Isolate tests by eliminating the use of the global sinon sandbox #25587

🏗 ✅ Isolate tests by eliminating the use of the global sinon sandbox #25587

Conversation

rsimha commented Nov 14, 2019 • edited

amp-owners-bot bot commented Nov 14, 2019

rsimha commented Nov 15, 2019 • edited

rsimha commented Nov 15, 2019

lannka left a comment

Choose a reason for hiding this comment

rsimha commented Nov 19, 2019 • edited

rsimha commented Nov 20, 2019 • edited

🏗 ✅ Isolate tests by eliminating the use of the global `sinon` sandbox #25587

🏗 ✅ Isolate tests by eliminating the use of the global `sinon` sandbox #25587

rsimha commented Nov 14, 2019 •

edited

rsimha commented Nov 15, 2019 •

edited

rsimha commented Nov 19, 2019 •

edited

rsimha commented Nov 20, 2019 •

edited