fix: timeouts due to audio, video elements #39
Conversation
Codecov Report
@@ Coverage Diff @@
## master #39 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 14 14
Lines 112 115 +3
Branches 11 10 -1
=========================================
+ Hits 112 115 +3
|
| // Disable preloading assets as it causes timeouts for audio/video elements | ||
| // with jest and delays webdriver tests by 2-3x when assets are not found (404) | ||
| // Ref: https://github.com/dequelabs/axe-core/issues/2528 | ||
| preload: false, |
There was a problem hiding this comment.
@trevor-bliss @cordeliadillon This one line is essentially the fix for audio/video timeout issue. Rest of the changes are updating tests, docs and bumping versions.
Can you please review this when possible. I will get a release to npm with this fix once the PR is merged.
packages/jest/README.md
Outdated
| - Not invoking it async would result in incorrect results e.g. no issues reported even when the page is not accessible | ||
| - **color-contrast**: Color-contrast check is disabled for Jest tests as it is [doesn't work in JSDOM](https://github.com/dequelabs/axe-core/issues/595) |
There was a problem hiding this comment.
Grammatical typo -- remove "is"
There was a problem hiding this comment.
Good catch, thanks - fixed
package.json
Outdated
| "lint:deps": "lerna exec depcheck", | ||
| "lint:depgraph": "yarn pkg:depgraph && git diff --quiet docs/sa11y_dependency_graph.svg || echo 'Dependency graph needs to be updated!'", | ||
| "lint:fix": "yarn lint --fix", | ||
| "lint:lockfile": "lockfile-lint --path yarn.lock --allowed-hosts registry.yarnpkg.com --validate-https", | ||
| "lint:staged": "lint-staged", | ||
| "lint:version": "vertioner && yarn lerna exec --since master vertioner", |
There was a problem hiding this comment.
It is a small util to check if the version field in package.json file in current branch is different than master. I am using it to ensure the versions for packages that are changed are bumped without fail.
There was a problem hiding this comment.
So the expectation is every PR must bump the version of all packages?
There was a problem hiding this comment.
Not really - only the changed packages (yarn lerna exec --since master .. takes care of that) and the monorepo root version.
There was a problem hiding this comment.
Ok interesting. I see a couple potential issues with this flow:
-
There can be version conflicts between PRs. Imagine there are several PRs open at the same time and they all bump the version to the next patch version. As soon as one gets merged, all other PRs must go in and bump their version again. This potentially repeats for every open PR when a different PR is merged.
-
There may be versions in github without corresponding NPM releases. Unless we release every commit to master to NPM, there may be several version bumps before a new version in NPM. Not necessarily a bad thing but can be confusing IMO.
-
We put the burden of versioning on the PR author. When I contribute to other projects I just focus on the code fix rather than worrying about whether something should be a patch vs. minor release.
Just my opinion but I think it makes more sense to leave the versions untouched in PRs and then do a release from master or a release branch and we as project maintainers decide when that release happens and what version to bump to.
There was a problem hiding this comment.
Thanks @trevor-bliss - Those are very valid concerns 👍
I have created a issue to track this and continue discussion #40
There was a problem hiding this comment.
@trevor-bliss I have removed the version check on mono-repo root due to a potential issue of it failing after merging into master
I was also able to remove force setting the master branch in CI branch since lerna exec seems to work fine without it.
Can you please give a glance and approve.
I will release to npm and work on improving versioning as part of #40
.circleci/config.yml
Outdated
| @@ -11,6 +11,8 @@ jobs: | |||
| - browser-tools/install-chrome | |||
| - browser-tools/install-chromedriver | |||
| - checkout | |||
| # CircleCI seems to mess with master ref which causes lint:version check to fail | |||
| - run: git branch -f master origin/master | |||
There was a problem hiding this comment.
I'm not sure about this. I don't really understand why we need vertioner to begin with though.
There was a problem hiding this comment.
Yeah agree, this is not ideal. But CircleCI seems to map local master branch to the current PR or something. Have been thinking about a better solution - will keep looking.
There was a problem hiding this comment.
This has been removed now.
as lint version with lerna seems to work fine without it
Related to audio, video elements cause timeout in jest even with audio/video rules disabled · Issue #2528 · dequelabs/axe-core
Fixes 🐛
Chores 🧹
Documentation 📝