-
-
Notifications
You must be signed in to change notification settings - Fork 640
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
Fix failing tests in the main branch #941
Conversation
Codecov Report
@@ Coverage Diff @@
## main #941 +/- ##
=======================================
Coverage 99.29% 99.29%
=======================================
Files 104 104
Lines 1554 1554
Branches 522 522
=======================================
Hits 1543 1543
Misses 11 11
|
ad78c86
to
3bac0f7
Compare
56b06de
to
5749b1b
Compare
5749b1b
to
1d4eac1
Compare
Finally found some time (and focus) on a flight to get back to this. |
1d4eac1
to
26e8e19
Compare
.npmrc
Outdated
@@ -1,3 +1,4 @@ | |||
package-lock=false | |||
allow-same-version=true | |||
message=v%s | |||
audit-level=low |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just testing a theory.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ljharb I'm going to need help with this one. It seems we're in a bind. We can't update semver
because of this package's commitment Node 4 support. Here are some options
- We could turn off the
posttest
checks for now, until the Node 4 is no longer supported in this project andsemver
can be updated. - Comment out the usages of
semver
in the project -- which means our test coverage for optional chaining is reduced -- and remove the dependency. - Update
semver
and drop Node 4 support. - Move the test cases that are wrapped in the
semver
check to their own folder and configure ESLint to only run on them if the version of ESLint is greater than v6.
I'm going to try #4, but I'm on a plane right now and internet is spotty, so please ignore some thrash in the PR as I use it to test out some theories.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't worry about the posttest tests due to the semver false positive CVE - if that's the only failure, i can take care of that. There's still a bunch of failing node tests tho.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, if you rebase on top of #944 instead of main, then that should address the problem :-) that way i can merge both PRs at once once the other tests are passing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will do
@@ -340,6 +339,7 @@ const neverValid = [ | |||
{ code: '<style onClick={() => {}} />;', errors: [expectedError] }, | |||
{ code: '<sub onClick={() => {}} />;', errors: [expectedError] }, | |||
{ code: '<sup onClick={() => {}} />;', errors: [expectedError] }, | |||
{ code: '<summary onClick={() => {}} />;', errors: [expectedError] }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if this test adjustment is right, this may be an issue in axobject-query
: A11yance/axobject-query#319
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dummdidumm ya, I think you're right. This is a tricky one. More details here: https://html.spec.whatwg.org/multipage/interactive-elements.html#the-summary-element
Given that we need to know the rendered output to make a determination about this element's role and behavior, the best course of action for a static linter is to be permissive of either state - an interactive element or a generic (static) element. I'll get this updated when I do a proper update of aria-query for ARIA 1.3.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dummdidumm #656 to tracks this.
@jessebeach - update on this? I'm waiting to update due to the semver being in I noticed it's in #944 also. Maybe we shouldn't combine the PRs and have #944 merged first? |
@karlnorling semver v5, v6, and v7 all have patches that fix that issue, so nobody should need to do anything except end users updating their lockfiles. |
4e7c7b9
to
b32d046
Compare
Looks like axe-core broke us too: dequelabs/axe-core#4127 |
b32d046
to
8d8f016
Compare
Last 2 failing tests seem to be about error messages for two tests showing up in the wrong order. I can't reproduce them locally, consistently. |
… kill the install process in npm < 6 See isaacs/jackspeak#4
…node This has the side benefit of making the error message ordering consistent, since when both were on the JSXOpeningElement, since eslint/node combinations ordered the errors inconsistently
@karlnorling, @ljharb merged it yesterday. |
There are a few failing tests in the main branch and this fixes them.
This PR also pegs
aria-query
to version 5.1.2.