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

chore: ensures axe Pro api #1836

Merged
merged 2 commits into from
Oct 9, 2019
Merged

chore: ensures axe Pro api #1836

merged 2 commits into from
Oct 9, 2019

Conversation

AutoSponge
Copy link
Contributor

@AutoSponge AutoSponge commented Oct 2, 2019

This adds tests that will help prevent breaking changes in axe-core that axe Pro depends on.

Closes issue: N/A (Q3 rock)

Reviewer checks

Required fields, to be filled out by PR reviewer(s)

  • Follows the commit message policy, appropriate for next version
  • Code is reviewed for security

@AutoSponge AutoSponge requested a review from a team as a code owner October 2, 2019 19:04
@AutoSponge AutoSponge changed the title chore: ensures axe Pro api [WIP] chore: ensures axe Pro api Oct 2, 2019
@AutoSponge AutoSponge force-pushed the axe-pro-api branch 2 times, most recently from 9ac1b33 to 534aaac Compare October 2, 2019 19:33
@AutoSponge AutoSponge changed the title [WIP] chore: ensures axe Pro api chore: ensures axe Pro api Oct 2, 2019
@straker
Copy link
Contributor

straker commented Oct 3, 2019

Question: Most of these checks are just checking that the API exists, is a function, and returns the correct thing. Should these be unit tests in the respected files rather than an integration test all in one?

Copy link
Member

@stephenmathieson stephenmathieson left a comment

Choose a reason for hiding this comment

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

I didn't dig in too far here, but think the test names are very confusing.

test/integration/api/external/index.js Outdated Show resolved Hide resolved
test/integration/api/external/index.js Outdated Show resolved Hide resolved
test/integration/api/external/index.js Outdated Show resolved Hide resolved
test/integration/api/external/index.js Outdated Show resolved Hide resolved
test/integration/api/external/index.js Outdated Show resolved Hide resolved
test/integration/api/external/index.js Outdated Show resolved Hide resolved
test/integration/api/external/index.js Outdated Show resolved Hide resolved
test/integration/api/external/index.js Outdated Show resolved Hide resolved
test/integration/api/external/index.js Outdated Show resolved Hide resolved
@AutoSponge AutoSponge dismissed stephenmathieson’s stale review October 3, 2019 20:47

better test names, removed console (tried to use Typscript'ish signature conventions)

@AutoSponge
Copy link
Contributor Author

AutoSponge commented Oct 3, 2019

Question: Most of these checks are just checking that the API exists, is a function, and returns the correct thing. Should these be unit tests in the respected files rather than an integration test all in one?

This file should be marked for special review once it lands, the same way we protect manifest changes in the extension. Changes in this file only could represent a shift in the axe-core api that breaks axe Pro. For that reason, spreading these tests into their respective modules would make it harder to keep an "eye" on the api axe Pro requires to operate.

test/integration/api/external/index.js Outdated Show resolved Hide resolved
@AutoSponge AutoSponge dismissed stephenmathieson’s stale review October 4, 2019 12:35

removed all "isFunction" checks

@WilcoFiers
Copy link
Contributor

@AutoSponge +1 on the decision to only test call signatures. We're testing those functions in depth elsewhere. I agree with the idea of adding you and/or Harris as code owner for these tests.

@stephenmathieson
Copy link
Member

Is there anything that needs to happen based on #1836 (comment)?

@AutoSponge
Copy link
Contributor Author

AutoSponge commented Oct 8, 2019

@stephenmathieson I assume a code owner of axe-core will need to update the reviewer settings when this file lands.

Should this PR include an update to .github/CODEOWNERS? We don't currently have an axe-pro owners group.

@stephenmathieson
Copy link
Member

I think this can land without a "codeowners" update and when @WilcoFiers's back, he can decide what to do moving forward.

@AutoSponge AutoSponge merged commit 5c7370a into develop Oct 9, 2019
@AutoSponge AutoSponge deleted the axe-pro-api branch October 9, 2019 17:11
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.

5 participants