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

Upgrade from nwsapi 2.2.5 to 2.2.6 started issue with JEST + RTL test cases #90

Open
sagar1111212121 opened this issue Jul 3, 2023 · 26 comments

Comments

@sagar1111212121
Copy link

sagar1111212121 commented Jul 3, 2023

Hello @dperini and team,

I found that upgrading nwsapi from 2.2.5 to 2.2.6 created an issue with the existing test cases written with JEST and RTL. it was all working fine in 2.2.5 just before sometime but as soon as, we did fresh npm install and almost 205 test cases stated failing which just worked fine in last couple of pipelines.

I dont know the exact reason for failures other than test log (Mostly around UserEvent.click and screen.getByTestId started failing)

We faced similar kind of issue when it was upgraded from 2.2.2 to 2.2.3 (Later 2.2.4 was released with quick fix which worked).

Is there anyone facing similar issue?

@sagar1111212121 sagar1111212121 changed the title Upgrade from nwsapi 2.2.5 to 2.2.6 started created issue with JEST test cases Upgrade from nwsapi 2.2.5 to 2.2.6 started created issue with JEST + RTL test cases Jul 3, 2023
@sagar1111212121 sagar1111212121 changed the title Upgrade from nwsapi 2.2.5 to 2.2.6 started created issue with JEST + RTL test cases Upgrade from nwsapi 2.2.5 to 2.2.6 started issue with JEST + RTL test cases Jul 3, 2023
@dperini
Copy link
Owner

dperini commented Jul 3, 2023

@sagar1111212121
thank you for reporting the issue.
I did run all my tests and jsdom/wpt test suite with no errors. So I don't know where to start looking for problems.
Could you kindly provide a test failing or send a log of the errors from your environment for me to check ?

@sagar1111212121
Copy link
Author

sagar1111212121 commented Jul 4, 2023

Hi @dperini

Thanks for the quick response. Please find below screenshots for your reference

image

image

image

There are almost 100 tests started failing because of above error. And all of these were working just fine before the upgrade.

@nhardy
Copy link

nhardy commented Jul 4, 2023

@dperini: I've traced the issue back to this line here:

'n=s.doc.activeElement;if(n===e)break;while(e&&(e=e.parentNode)){if(n===e)break;}' +

It looks like if(n===e)break; appears outside the while loop which causes this syntax error.

@sagar1111212121
Copy link
Author

sagar1111212121 commented Jul 4, 2023

For a quick workaround to unblock the pipeline, I have added below code in my 'setupTests.ts' and it is working fine for time being but we will have to find the proper solution.

image

@mamang126
Copy link

Also another fix is override the version of the peer dependency:

"devDependencies": {
  "nwsapi": "2.2.5"
},
"overrides": {
  "nwsapi": "$nwsapi"
}

@dperini
Copy link
Owner

dperini commented Jul 4, 2023

@sagar1111212121
I believe I did find out the reason of the regression. Your suggested point of failure seems to be correct.
This is what happened ... The line that you pointed out existed before as part of the ":focus-within" resolution.
In the last 2.2.6 release I fixed the checking order of ":focus-within" to happen before the ":focus" pseudo-class.
However since the ":focus-within" pseudo-class was never tested thoroughly, now it is part of the accepted selectors.
Unfortunately the resolver for that was never tried nor executed. That's the reason of the current bug.
I am already working on the patch, I hope to be able to fix this today.

Thank you for raising the alert on this and for giving me a starting point about where to look for the fix.

@doberkofler
Copy link

@sagar1111212121 I'm also having this problem and explicitly adding a dependency can be used as a workaround

@nandeshwarshubh
Copy link

We are also facing the same issue, downgrading to 2.2.5 worked.

@dperini
Copy link
Owner

dperini commented Jul 4, 2023

@sagar1111212121
please check the fixes I did commit today in this repository.
I will wait for a few confirmations and do minor release 2.2.7 tomorrow.

@alvarogfn
Copy link

For anyone using yarn, this workaround worked for me:

"resolutions": {
  "**/nwsapi": "2.2.5"
}

@val1984
Copy link

val1984 commented Jul 5, 2023

For anyone using pnpm, put this in your package.json file until 2.2.7 is released:

{
  "pnpm": {
    "overrides": {
      "nwsapi": "2.2.5"
    }
  }
}

@isc30
Copy link

isc30 commented Jul 5, 2023

hi, im also getting this issue when using jest 29.6.0 and jsdom 22.1.0

@dep
Copy link

dep commented Jul 5, 2023

@sagar1111212121

Thanks for the quick response. Please find below screenshots for your reference

Thanks for reporting this. Please consider pasting your error message as text instead of images so folks Googling the same error message can find pages like these. 🙏🏻

@aishwarya-tw
Copy link

aishwarya-tw commented Jul 6, 2023

@dperini We are also facing this issue and it is blocking our release. Unfortunately we can't use the workaround in our case. Any idea when 2.2.7 will be released with the fix?

@dperini
Copy link
Owner

dperini commented Jul 6, 2023

@aishwarya-tw and all affected users.
Sorry for the troiubles I just pushed the latest 2.2.7 release on NPM.
Please confirm that the ':focus-within' pseudo-class related regression is fixed.

@sagar1111212121
Copy link
Author

sagar1111212121 commented Jul 6, 2023

@dperini Thanks for the quick action

I am checking it now and should be able to update in sometime.

@sagar1111212121
Copy link
Author

@sagar1111212121

Thanks for the quick response. Please find below screenshots for your reference

Thanks for reporting this. Please consider pasting your error message as text instead of images so folks Googling the same error message can find pages like these. 🙏🏻

Yah, Didn't realize this. But will keep this in mind :)

@aishwarya-tw
Copy link

@aishwarya-tw and all affected users. Sorry for the troiubles I just pushed the latest 2.2.7 release on NPM. Please confirm that the ':focus-within' pseudo-class related regression is fixed.

@dperini Thank you so much for the quick action and timely release! 🙏🏼 I can confirm that the fix is working for us and our build pipeline is running all the way through!

@sagar1111212121
Copy link
Author

@dperini

@aishwarya-tw and all affected users. Sorry for the troiubles I just pushed the latest 2.2.7 release on NPM. Please confirm that the ':focus-within' pseudo-class related regression is fixed.

@dperini - You rock.

All 1400 test cases passed without any issue now :)

Thanks for the quick resolution.

@vonny29
Copy link

vonny29 commented Jul 6, 2023

@dperini thank you so much for quick response, all my test cases are working again now. :)

@dperini
Copy link
Owner

dperini commented Jul 6, 2023

@sagar1111212121 @val1984 @vonny29 and all users of "nwsapi" ...
Thank you for the collaboration, this allows for quick response time.
Just notifying me if it works or if it doesn't work, is an appreciated help.
Next step would be to provide a minimal test showing the error you get.

I am now tackling ':focus-visible' pseudo-class which in my tests is already working well (targeting release 2.2.8).
Then there is a WIP for including WPT as the base unit test for "nwsapi", this will grant less regression like this last one.
In the working queue I also have the resolver of the following pseudo-classes (if they make sense & there is demand for):

rsrc_state: '(playing|paused|seeking|buffering|stalled|muted|volume-locked)' - (media resources state)
disp_state: '(open|closed|modal|fullscreen|picture-in-picture)' - (element display state)
time_state: '(current|past|future)' - (time dimensions)

and, on request, in the queue I also have an easy one like the :defined pseudo-class which have been requested by some.
Thank you all again.

@lakshman0369
Copy link

@dperini
for us the below assertion is failing after upgrading the nwsapi from 2.2.5 to 2.2.7. When we fixed the version to 2.2.5 this assertion is working fine.
expect(screen.getByRole('button')).toHaveStyle( 'background-color: transparent' );

image

When ever the component is rendered, it is taking the active state background-color even though it isn't set to active. Please look into this issue.

@dperini
Copy link
Owner

dperini commented Jul 13, 2023

@lakshman0369
I will have a look in to this issue.
Thank you for reporting.

@Xe11o
Copy link

Xe11o commented Jul 14, 2023

Reporting similar issue to the above

Assertion:

expect(renderedTabs[0]).toHaveStyle(`
  color: rgb(0, 82, 204);
  background-color: white;
`);

CSS:
Screenshot 2023-07-14 at 19 55 39

It fails because during the test background-color has the hover rgb value rather than the white value it is expected to have. Hover / active styles seem to be getting applied erroneously during the test.

2.2.5 is good but this problem starts on 2.2.6.

@dperini
Copy link
Owner

dperini commented Jul 15, 2023

@lakshman0369 @Xe11o
please see commit d8ac965 it should be fixing the issue.
I saw it was related to :hover and :active pseudo-classes in your reports.

@bazanowsky
Copy link

Hi @dperini
indeed this commit fixes the issue - I saw you already merged it to master - but the package is not updated on npm - using version 2.2.7 still have this bug.

When you expect to publish this fix it to npm?

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

No branches or pull requests