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

refactor: use new test-fixtures path #880

Merged
merged 2 commits into from
Oct 20, 2023
Merged

refactor: use new test-fixtures path #880

merged 2 commits into from
Oct 20, 2023

Conversation

straker
Copy link
Contributor

@straker straker commented Oct 18, 2023

Use the new exported fixturesPath from axe-test-fixtures and remove all the symlinks to external and all fixtures directories.

@straker straker requested a review from a team as a code owner October 18, 2023 20:53
@@ -17,6 +17,7 @@
"devDependencies": {
"@typescript-eslint/eslint-plugin": "^5.40.0",
"@typescript-eslint/parser": "^5.40.0",
"axe-test-fixtures": "github:dequelabs/axe-test-fixtures#v1",
Copy link
Contributor Author

@straker straker Oct 18, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This wasn't pinned to v1 so wasn't getting the path update. Also not sure why it was a dependency so made it a dev dependency.

Copy link
Contributor

@WilcoFiers WilcoFiers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small comment, please have a look

@@ -619,7 +616,7 @@ describe('@axe-core/webdriverjs', () => {
});

it('with chaining include and exclude', async () => {
await driver.get(`${addr}/context.html`);
await driver.get(`${addr}/context-include-exclude.html`);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test doesn't really prove anything. If you only had the .includes the assertions would still pass because the excludes aren't nested in an include. Not sure that needs to be fixed in this PR, but something should be done about it IMO.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup. Noticed that too. This would fix it dequelabs/axe-test-fixtures#28

Copy link
Contributor

@Zidious Zidious left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@straker straker merged commit a80add3 into develop Oct 20, 2023
27 checks passed
@straker straker deleted the test-fixtures branch October 20, 2023 16:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants