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

Skip flaky inabox tests #20682

Merged
merged 2 commits into from
Feb 5, 2019
Merged

Skip flaky inabox tests #20682

merged 2 commits into from
Feb 5, 2019

Conversation

cvializ
Copy link
Contributor

@cvializ cvializ commented Feb 5, 2019

@lannka
Copy link
Contributor

lannka commented Feb 5, 2019

@cvializ is it only failing on SauceLabs? Could you do skipSafari().skipFirefox(). ?

@lannka
Copy link
Contributor

lannka commented Feb 5, 2019

/cc @zombifier , as you've been doing a lot of flaky test debugging on SL recently. Do you have any insights?

@zombifier
Copy link
Contributor

zombifier commented Feb 5, 2019

I have been looking for a while now. Something changed in Firefox 65 that caused it to fail to load the ads.localhost domain during testing on Travis. 64 and prior (along with Safari) works fine for some reason. Friendly and safe frame cases are still fine since they don't need to load from a localhost subdomain.

Subdomaining localhost only works on Chrome out of the box, and requires modifying the hosts file for other browsers to work (you need to do this if you want to run the inabox tests locally on a non-Chrome browser). Maybe something is missing in the Travis environment for Firefox 65? I can't tell at the moment.

EDIT: Just to be clear, Firefox 65 on local testing passes if I set /etc/hosts so ads.localhost is properly redirected. This is the case in prior versions as well.

@cvializ
Copy link
Contributor Author

cvializ commented Feb 5, 2019

@aghassemi @lannka PTAL I disabled the test on Firefox

@lannka
Copy link
Contributor

lannka commented Feb 5, 2019

Firefox 65 on local testing passes if I set /etc/hosts so ads.localhost is properly redirected. This is the case in prior versions as well.

thanks @zombifier I think this is the root cause as @torch2424 also found out.

On Travis we did set hosts names. Not sure about SauceLabs. @rsimha do you know?

@rsimha
Copy link
Contributor

rsimha commented Feb 5, 2019

@lannka According to this article:

Editing the Host file of the virtual machine will not work if Sauce Connect Proxy is in use. If you are using Sauce Connect Proxy, the Host file of the machine running Sauce Connect Proxy will be referenced and you can make the desired changes there.

We do use the Sauce Connect Proxy on Travis, so I'd imagine the host file of the Travis VM will be referenced. Is this what you're observing?

@cvializ cvializ merged commit 3d8ae95 into ampproject:master Feb 5, 2019
@cvializ
Copy link
Contributor Author

cvializ commented Feb 5, 2019

Merging now to unblock other PRs. Can revert when hosts file solution is implemented.

@rsimha
Copy link
Contributor

rsimha commented Feb 5, 2019

To add, the Travis documentation for the hosts addon says:

If your build requires custom hostnames, you can specify a single host or a list of them in your .travis.yml. Travis CI will automatically configure the hostnames in /etc/hosts and resolve them to 127.0.0.1.

We're already doing this for ads.localhost via this line in .travis.yml:

amphtml/.travis.yml

Lines 36 to 37 in 2b83339

hosts:
- ads.localhost

Is this sufficient for our purpose? Are we not seeing the desired effect on Firefox 65?

@lannka
Copy link
Contributor

lannka commented Feb 5, 2019

I just tested FF64, it also requires /etc/hosts to load ads.localhost. So not sure what is changed in FF65 that caused ads.localhost fail to load on SL.

@cvializ cvializ deleted the flake/inabox branch February 7, 2019 18:46
nbeloglazov pushed a commit to nbeloglazov/amphtml that referenced this pull request Feb 12, 2019
* Skip flaky inabox tests

* Skip firefox instead
noranazmy pushed a commit to noranazmy/amphtml that referenced this pull request Mar 22, 2019
* Skip flaky inabox tests

* Skip firefox instead
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.

6 participants