Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

🐛 Re-enable Safari on Sauce labs #15510

Merged
merged 1 commit into from
May 25, 2018
Merged

🐛 Re-enable Safari on Sauce labs #15510

merged 1 commit into from
May 25, 2018

Conversation

rsimha
Copy link
Contributor

@rsimha rsimha commented May 23, 2018

From Sauce Labs Help Center:

We've done some fixes on our backend that might help with the current behavior you're seeing.
Could you try running your suite again and tell us if you see any improvement on the error rate?
If you don't please send us any console logs and build links so we can have a closer look.

Fixes #14848

@rsimha rsimha self-assigned this May 23, 2018
@rsimha rsimha requested a review from dreamofabear May 23, 2018 01:18
@rsimha
Copy link
Contributor Author

rsimha commented May 23, 2018

/to @choumx

I'll re-rerun Travis a few times to see if this is indeed fixed.

@dreamofabear dreamofabear changed the title 🐛 Re-enable testing on Sauce labs 🐛 Re-enable Safari on Sauce labs May 23, 2018
@rsimha
Copy link
Contributor Author

rsimha commented May 24, 2018

Trying the fix in https://support.saucelabs.com/hc/en-us/articles/115010079868

@erwinmombay Since Safari doesn't play well with the localhost name, the suggestion is for karma to use a different hostname like 127.0.0.1. Any idea how to get our tests to play well with this? I tried browserify-replace, but there's no way to reliably fix all instances of hardcoded localhost in our tests.

Does this mean we're effectively unable to test on Safari?

Results should show up in https://travis-ci.org/ampproject/amphtml/builds/383459381

@rsimha
Copy link
Contributor Author

rsimha commented May 25, 2018

@choumx This has passed three times. Latest run: https://travis-ci.org/ampproject/amphtml/builds/383874710

I've re-enabled Safari for unit tests, and Safari and Android browser for integration tests. Looking good after multiple tries.

The iphone config seems to have changed, so I'll enable that in a separate PR.

Merging (fingers crossed)...

@rsimha rsimha merged commit a63b728 into ampproject:master May 25, 2018
@rsimha rsimha deleted the 2018-05-22-EnableSauce branch May 25, 2018 22:18
@lannka
Copy link
Contributor

lannka commented Jun 1, 2018

hostname: process.platform === 'darwin' ? '127.0.0.1' : 'localhost',

This change broke test-amp-analytics.js when running locally, where some tests are doing URL comparison with http://localhost/ .

Having this discrepancy between environments can be a bug hole. Can we unify to 127.0.0.1 for instance?

@dreamofabear
Copy link

This also breaks test-impression.js locally.

FAILED TESTS:
  impression
    clickUrl
      ● should replace location href only with query params
        HeadlessChrome 0.0.0 (Mac OS X 10.13.4)
      AssertionError: expected 'http://127.0.0.1:9876/context.html?bar=foo&test=4321&gclid=123456&foo=bar&example=123' to equal 'http://localhost:9876/context.html?bar=foo&test=4321&gclid=123456&foo=bar&example=123'
          at /var/folders/xy/zdjxmyvn1vjg75zhtgc5r28h006wch/T/test/functional/test-impression.js:264:40 <- /var/folders/xy/zdjxmyvn1vjg75zhtgc5r28h006wch/T/aa74d3bbeb793a22ab3bc8e8dbdee15b.browserify:73683:41

    replaceUrl
      ● should use init replaceUrl parm if viewer has no capability
        HeadlessChrome 0.0.0 (Mac OS X 10.13.4)
      AssertionError: expected 'http://127.0.0.1:9876/context.html?bar=foo&test=4321&gclid=123456&foo=bar&example=123' to equal 'http://localhost:9876/v/s/f.com/?gclid=1234&amp_js_v=1&init'
          at /var/folders/xy/zdjxmyvn1vjg75zhtgc5r28h006wch/T/test/functional/test-impression.js:291:40 <- /var/folders/xy/zdjxmyvn1vjg75zhtgc5r28h006wch/T/aa74d3bbeb793a22ab3bc8e8dbdee15b.browserify:73721:41

      ● should replace location href with replaceUrl from viewer
        HeadlessChrome 0.0.0 (Mac OS X 10.13.4)
      AssertionError: expected 'http://127.0.0.1:9876/context.html?bar=foo&test=4321&gclid=123456&foo=bar&example=123?bar=foo&test=4321#hash' to equal 'http://localhost:9876/v/s/f.com/?gclid=1234&amp_js_v=1#hash'
          at /var/folders/xy/zdjxmyvn1vjg75zhtgc5r28h006wch/T/test/functional/test-impression.js:339:40 <- /var/folders/xy/zdjxmyvn1vjg75zhtgc5r28h006wch/T/aa74d3bbeb793a22ab3bc8e8dbdee15b.browserify:73779:41

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Karma tests disconnect when running tests on Safari
4 participants