search

pa11y / pa11y-ci

Pa11y CI is a CI-centric accessibility test runner, built using Pa11y
https://pa11y.org
GNU Lesser General Public License v3.0
519 stars 63 forks source link

Remove integration tests dependence on DNS failures #132

Closed josebolos closed 3 years ago

josebolos commented 3 years ago

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 commented 3 years ago

@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 commented 3 years ago

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