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

Remove integration tests dependence on DNS failures #132

Closed
josebolos opened this issue Apr 22, 2021 · 2 comments · Fixed by #133
Closed

Remove integration tests dependence on DNS failures #132

josebolos opened this issue Apr 22, 2021 · 2 comments · Fixed by #133

Comments

@josebolos
Copy link
Member

The replacement of Travis with GitHub Actions for CI is failing, apparently due to github doing something funky with the DNS that is causing tests to fail unexpectedly.

A number of integration tests rely of puppeteer failing to resolve hostnames like notahost in order to verify that errors during the test running are propagated properly. This reliance on the hostname failing to resolve correctly goes against best practices, as there are a number of reasons why this may not be the case, and the domain may resolve successfully.

We have an example in pa11y, where a similar test was already disabled:
https://github.com/pa11y/pa11y/blob/d602acaad1db8ed90f55ab59cf73ebb3e782c768/test/integration/cli/exit-codes.test.js#L33

We need to remove all the testing that relies on external factors like the DNS failing to resolve a hostname, so we can continue to make changes to pa11y-ci. This is currently blocking any development or migration work.

@aarongoldenthal
Copy link
Contributor

@josebolos

I took a look at options, trying to find cases that did not rely on external dependencies, but failed to execute. They obviously also had to work given the URL sanitization that pa11y and pa11y-ci perform (e.g. puppeteer passes asdfasdf as the URL for their DNS failure test, but pa11y corrects that to http://asdfasdf). I came up with 3 cases:

Pass no url

If either an empty string or http:// is passed to pa11y, it gets sent to puppeteer as http://, which fails with Protocol error (Page.navigate): Cannot navigate to invalid URL. This could work for the URL cases, but seems less definitive when testing the output.

Pass a URL with an invalid domain

If a URL with an invalid domain is used, e.g. http://-invalidurl.com/erroring-1 (whith a domain that begins with "-", which is invalid), it will never be resolved. In addition, Chrome appears to identify it as invalid without making a DNS requests and consistently returns ERR_NAME_NOT_RESOLVED. That is not the case universally as other browsers and tools do make a DNS request (I tested Firefox, Edge/Chromium, and curl, which all made a DNS request). The DNS request will always fail in any case since it is invalid, but as can be seen with the current Actions failures it's possible DNS could return a different failure message. So, this works with puppeteer/Chrome without any external dependencies, but seemed to rely on a feature that could change in Chrome, or could cause issues later if changes were made to test puppeteer with other browsers (e.g. Firefox), move to something like Playwright, etc.

Pass a URL with an invalid file name

This is based on the example already used for the CLI paths test, but passing an invalid file URL, e.g. ./foo/erroring.html, results in ERR_FILE_NOT_FOUND with no external dependencies. It's pretty much a one-for-one swap with url/error, but the JSON test needs to be updated to include the file:// url with the absolute path at runtime. This seemed like the best option to remove external dependencies.

@josebolos
Copy link
Member Author

This is great! Thank you so much for picking this one up, @aarongoldenthal. It's very very much appreciated.

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

Successfully merging a pull request may close this issue.

2 participants