Run unit tests on Chrome and Safari via Sauce Labs #11827

rsimha · 2017-10-27T16:12:19Z

With this PR, in addition to running all unit tests on Chrome (latest), we run a subset of the unit tests on Safari (latest) via Sauce Labs.

We also add Safari 10 and 11 to the mix for integration tests.

Fixes #6039

/to @choumx @erwinmombay @aghassemi

dreamofabear · 2017-10-27T19:17:06Z

Is there value in running tests on Saucelabs + Chrome latest? I'm not sure we're actually getting additional signal here.

rsimha · 2017-10-27T19:19:09Z

@choumx This is a prequel to the PRs that will enable the unit tests on non-chrome-latest browsers. Right now, there are a bunch of unit tests that fail when run on other browsers, so I'm going to have to do that clean up in a separate PR.

dreamofabear · 2017-10-27T19:22:55Z

So we'd disable latest-Chrome once we add support for other browsers?

I guess it's not obvious to me that #6039 is a good idea in the first place. Will we need to start transpiling test code to get it to run on browsers other than latest Chrome?

rsimha · 2017-10-27T19:31:05Z

@choumx You've raised a good point :)

We wouldn't disable unit tests on latest chrome on sauce labs, since this PR will no longer run unit tests on the version of chrome that's native to Travis. The idea is to (soon) run unit tests on latest chrome, latest safari, and latest chrome android.

I don't believe we'll need to transpile the tests on a couple of the latest browsers. The intention isn't to run unit tests on the entire sauce labs browsers.

@aghassemi can comment on the rationale underlying his original issue, after which we can determine if this PR is still a good idea.

aghassemi · 2017-10-30T16:30:52Z

@choumx @choumx The rational behind #6039 is that we had a few production bugs due to code using web platforms API's not available in Safari. Unit tests would have caught those bugs but they do not run on Safari. So getting unit tests run in latest Safari same way we do in latest Chrome is the important part of this. Other browsers, older versions is a nice to have.

dreamofabear · 2017-10-30T18:43:23Z

@aghassemi Integration testing + presubmit bans for APIs with cross-browser issues seems better IMO. We already have infra for both and we won't need to polyfill/transpile test code.

rsimha · 2017-10-30T18:43:23Z

@choumx @aghassemi As of now, there are ~20 failures when unit tests are run on Safari: https://travis-ci.org/ampproject/amphtml/jobs/293802513

I expect that with a few fixes, we should be able to run unit tests on Safari for all PRs (or at least for push builds). Since tests on different browsers are run in parallel, I expect that the overall increase in build time should be small relative to the increase in the number of tests we run.

Edit: I typed this before the reply above. Will hold off until we're sure of the direction we want to pursue.

rsimha · 2017-10-31T17:54:57Z

@aghassemi @choumx Is there a list of APIs with cross-browser issues? Would it make sense to ban those APIs via presubmit checks instead of running unit tests on Safari?

aghassemi · 2017-10-31T18:15:25Z

@rsimha-amp @choumx The idea behind running unit tests in Safari was to "catch" these cross-browser issues that devs initially didn't realize is an issue and hence is not banned in presubmit. After we know they are an issue, then yes banning them is the right action. I see having unit tests run in Safari as just anther safety check to increases chances of catching issues we are not aware of.

rsimha · 2017-10-31T23:22:28Z

@aghassemi @choumx I've gone ahead and added a new commit to this PR that fixes / skips the individual tests that currently fail on Safari, and assigned TODOs to the owners of each of those tests. I've also enabled unit test runs on Chrome and Safari by default for PR runs. Hopefully, the next Travis PR check build should be green.

When I looked at the overall timing of the build, there wasn't a significant increase in duration, due to the fact that tests are run in parallel on the two browsers. With this, I'm hoping both your concerns should be addressed.

PTAL.

Tip: Use this link for review, so that whitespace-only changes due to indentation are hidden: https://github.com/ampproject/amphtml/pull/11827/files?w=1

ampprojectbot · 2017-11-06T21:04:46Z

This issue doesn't have a category which makes it harder for us to keep track of it. @rsimha-amp Please add an appropriate category.

aghassemi · 2017-11-09T00:39:08Z

@rsimha-amp unit tests are still failing, will review after green?

rsimha · 2017-11-14T17:37:42Z

@aghassemi Running a subset of the tests on Safari works. See https://travis-ci.org/ampproject/amphtml/jobs/302595652

This is ready for a full review. You can use https://github.com/ampproject/amphtml/pull/11827/files?w=1 to filter out whitespace-only changes while reviewing.

rsimha · 2017-11-15T18:29:44Z

@aghassemi Ping

rsimha · 2017-11-15T20:06:41Z

@erwinmombay could you take a look? I have a couple other PRs that are waiting for this to be merged.

aghassemi · 2017-11-15T22:23:59Z

test/functional/test-custom-element.js

-    resumeCallback() {
-      testElementResumeCallback();
+  // TODO(dvoytenko): Make this test work on Safari. See #11827.
+  describe.configure().skipSafari().run('CustomElement', () => {


GitHub UI doesn't make it clear what actually changed here. Is there anything changes other than skipSafari?

No, this just adds the skipSafari.

Did you click the link I sent you (https://github.com/ampproject/amphtml/pull/11827/files?w=1)? Adding ?w=1 will filter out whitespace changes.

Whoa! had no idea about ?w=1 stealing it for the next time I am assigned tip of the week.

aghassemi · 2017-11-15T22:24:03Z

test/functional/test-ampdoc.js

@@ -166,7 +166,8 @@ describe('AmpDocService', () => {
      }).to.throw(/The shadow root already contains ampdoc/);
    });

-    it('should navigate via host', () => {
+    // TODO(dvoytenko): Make this test work on Safari. See #11827.


nit: // TODO(dvoytenko, #11827) (Dima has a tool that extracts these if the TODO(username, #issue) format is followed.

rsimha self-assigned this Oct 27, 2017

rsimha requested review from erwinmombay, aghassemi and dreamofabear October 27, 2017 16:12

googlebot added the cla: yes label Oct 27, 2017

rsimha mentioned this pull request Oct 27, 2017

Run unit tests on more browsers #6039

Closed

4 tasks

danielrozenberg approved these changes Oct 27, 2017

View reviewed changes

ampprojectbot added this to the Pending Triage milestone Oct 30, 2017

rsimha modified the milestones: Pending Triage, Sprint H1 November Oct 31, 2017

rsimha changed the title ~~Run unit tests on a subset of Sauce labs browsers~~ Run unit tests on Chrome and Safari via Sauce Labs Oct 31, 2017

rsimha mentioned this pull request Nov 1, 2017

Weekly Status 2017-10-30 #11876

Closed

ampprojectbot added P2: Soon Type: Bug labels Nov 6, 2017

dreamofabear added the WG: infra label Nov 7, 2017

rsimha mentioned this pull request Nov 14, 2017

Percy <-> Github integration is not picking up the latest Percy build for a PR, thereby blocking it from being merged percy/percy-client#30

Closed

Run unit tests on a subset of Sauce labs browsers

a374f3b

rsimha added 7 commits November 15, 2017 10:50

Update list of browsers for saucelabs_lite

02b9981

Skip tests that fail on Sauce labs

20e4b71

Add a TODO to add more browsers to saucelabs_lite

18cec76

Fix / skip unit tests that fail on Safari

c785188

Run subset of unit tests on Safari

097aaa9

Revert changes to extensions/**/test/*

859e61f

Print start message for local runs too

6d6f897

aghassemi approved these changes Nov 15, 2017

View reviewed changes

Address comments

6a1a04a

rsimha merged commit 1f605cc into ampproject:master Nov 16, 2017

rsimha deleted the 2017-10-24-SauceBrowsers branch November 16, 2017 00:31

riklund pushed a commit to riklund/amphtml that referenced this pull request Nov 16, 2017

Run unit tests on Chrome and Safari via Sauce Labs (ampproject#11827)

1bd4643

neko-fire pushed a commit to 3qnexx/amphtml that referenced this pull request Nov 17, 2017

Run unit tests on Chrome and Safari via Sauce Labs (ampproject#11827)

c4ab0fc

rsimha removed this from the Sprint H1 November milestone Nov 17, 2017

ghost pushed a commit that referenced this pull request Dec 6, 2017

Run unit tests on Chrome and Safari via Sauce Labs (#11827)

e01cdf4

gzgogo pushed a commit to gzgogo/amphtml that referenced this pull request Jan 26, 2018

Run unit tests on Chrome and Safari via Sauce Labs (ampproject#11827)

0a74f19

rsimha mentioned this pull request May 21, 2019

🏗 Run unit tests on Firefox #22420

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Run unit tests on Chrome and Safari via Sauce Labs #11827

Run unit tests on Chrome and Safari via Sauce Labs #11827

rsimha commented Oct 27, 2017 •

edited

Loading

dreamofabear commented Oct 27, 2017

rsimha commented Oct 27, 2017

dreamofabear commented Oct 27, 2017

rsimha commented Oct 27, 2017

aghassemi commented Oct 30, 2017

dreamofabear commented Oct 30, 2017

rsimha commented Oct 30, 2017 •

edited

Loading

rsimha commented Oct 31, 2017

aghassemi commented Oct 31, 2017

rsimha commented Oct 31, 2017 •

edited

Loading

ampprojectbot commented Nov 6, 2017

aghassemi commented Nov 9, 2017

rsimha commented Nov 14, 2017 •

edited

Loading

rsimha commented Nov 15, 2017

rsimha commented Nov 15, 2017

aghassemi Nov 15, 2017

rsimha Nov 16, 2017 •

edited

Loading

aghassemi Nov 16, 2017

aghassemi Nov 15, 2017

rsimha Nov 16, 2017

Run unit tests on Chrome and Safari via Sauce Labs #11827

Run unit tests on Chrome and Safari via Sauce Labs #11827

Conversation

rsimha commented Oct 27, 2017 • edited Loading

dreamofabear commented Oct 27, 2017

rsimha commented Oct 27, 2017

dreamofabear commented Oct 27, 2017

rsimha commented Oct 27, 2017

aghassemi commented Oct 30, 2017

dreamofabear commented Oct 30, 2017

rsimha commented Oct 30, 2017 • edited Loading

rsimha commented Oct 31, 2017

aghassemi commented Oct 31, 2017

rsimha commented Oct 31, 2017 • edited Loading

ampprojectbot commented Nov 6, 2017

aghassemi commented Nov 9, 2017

rsimha commented Nov 14, 2017 • edited Loading

rsimha commented Nov 15, 2017

rsimha commented Nov 15, 2017

aghassemi Nov 15, 2017

Choose a reason for hiding this comment

rsimha Nov 16, 2017 • edited Loading

Choose a reason for hiding this comment

aghassemi Nov 16, 2017

Choose a reason for hiding this comment

aghassemi Nov 15, 2017

Choose a reason for hiding this comment

rsimha Nov 16, 2017

Choose a reason for hiding this comment

rsimha commented Oct 27, 2017 •

edited

Loading

rsimha commented Oct 30, 2017 •

edited

Loading

rsimha commented Oct 31, 2017 •

edited

Loading

rsimha commented Nov 14, 2017 •

edited

Loading

rsimha Nov 16, 2017 •

edited

Loading