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
520 stars 63 forks source link

Default configuration section in Readme has wrong key name #91

Closed jamesbarnett91 closed 4 years ago

jamesbarnett91 commented 4 years ago

A recent PR (https://github.com/pa11y/pa11y-ci/pull/89) changed the json key of the default configuration option from defaults to default. As far as I can tell this is incorrect? Config options are not picked up if specified under the default key. Using the previous value of defaults works correctly however.

For example, according to the current documentation this should set the timeout to 1ms (and therefore immediately timeout as an easily reproducible test case). But the config is not picked up, and the request works fine

{
  "default": {
    "timeout": 1
  },
  "urls": [
    "http://pa11y.org/",
    "http://pa11y.org/contributing"
  ]
}

Using the previous key of defaults works fine however, and the short timeout is correctly picked up here (and so the request immediately timeout).

{
  "defaults": {
    "timeout": 1
  },
  "urls": [
    "http://pa11y.org/",
    "http://pa11y.org/contributing"
  ]
}

See https://github.com/pa11y/pa11y-ci/blob/master/bin/pa11y-ci.js#L192 which is referencing config.defaults. The integration tests also still reference defaults. See https://github.com/pa11y/pa11y-ci/blob/master/test/integration/mock/config/defaults.json

Tested using pally-cli 2.3.0

Could the documentation change please be reverted? Or the code updated to look for the default key.

Thanks for a very useful tool!

einSelbst commented 4 years ago

Interesting. I once followed the readme and then had the problem mentioned in #85. Because changing "defaults" to "default" did fix it at the time for me I submitted #89. I haven't yet tested with "defaults" again.

fam4r commented 4 years ago

defaults is correct.

Why merging a PR which is wrong? The tests using default totally do not work.

einSelbst commented 4 years ago

To make it clear, the PR #89 only changed the documentation in the Readme, not the actual code, so I think the description here is a bit misleading.

fam4r commented 4 years ago

Hi @einSelbst,

yes, ok, #89 only changes documentation in the README (which is the only one documentation available now), but people that want to use pa11y-ci may be conducted to errors because of a wrong documentation (the code, in fact, accesses to defaults variable and so .pa11yci config file default options are being succesfully parsed only if using the defaults JSON key).

For instance, that was exactely my case: i used default because of the documentation and it didn't work; only looking at this and other issues (which reported examples with the defaults key) helped me to find out the error.

einSelbst commented 4 years ago

FWIW a made the PR in the best intention, especially to prevent the situation you experienced, which I, kind of, experienced before as I mentioned regarding #85 .

I'm totally fine if it is reverted and in fact just commented to explain my original intention to make it even easier to do so.