nwsapi icon indicating copy to clipboard operation
nwsapi copied to clipboard

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

Open sagar1111212121 opened this issue 2 years ago • 26 comments

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 avatar Jul 03 '23 18:07 sagar1111212121

@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 ?

dperini avatar Jul 03 '23 20:07 dperini

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.

sagar1111212121 avatar Jul 04 '23 00:07 sagar1111212121

@dperini: I've traced the issue back to this line here: https://github.com/dperini/nwsapi/blob/32da18d5ca9c39057ff8083c87ae0ebf58dbe977/src/nwsapi.js#L1087

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

nhardy avatar Jul 04 '23 02:07 nhardy

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

sagar1111212121 avatar Jul 04 '23 03:07 sagar1111212121

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

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

mamang126 avatar Jul 04 '23 09:07 mamang126

@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.

dperini avatar Jul 04 '23 09:07 dperini

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

doberkofler avatar Jul 04 '23 11:07 doberkofler

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

nandeshwarshubh avatar Jul 04 '23 14:07 nandeshwarshubh

@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.

dperini avatar Jul 04 '23 20:07 dperini

For anyone using yarn, this workaround worked for me:

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

alvarogfn avatar Jul 04 '23 22:07 alvarogfn

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

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

val1984 avatar Jul 05 '23 08:07 val1984

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

isc30 avatar Jul 05 '23 09:07 isc30

@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. 🙏🏻

dep avatar Jul 05 '23 18:07 dep

@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?

aishwarya-tw avatar Jul 06 '23 06:07 aishwarya-tw

@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 avatar Jul 06 '23 09:07 dperini

@dperini Thanks for the quick action

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

sagar1111212121 avatar Jul 06 '23 09:07 sagar1111212121

@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 :)

sagar1111212121 avatar Jul 06 '23 09:07 sagar1111212121

@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!

aishwarya-tw avatar Jul 06 '23 09:07 aishwarya-tw

@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.

sagar1111212121 avatar Jul 06 '23 09:07 sagar1111212121

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

vonny29 avatar Jul 06 '23 12:07 vonny29

@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.

dperini avatar Jul 06 '23 13:07 dperini

@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.

lakshman0369 avatar Jul 11 '23 14:07 lakshman0369

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

dperini avatar Jul 13 '23 13:07 dperini

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.

Xe11o avatar Jul 14 '23 18:07 Xe11o

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

dperini avatar Jul 15 '23 22:07 dperini

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?

bazanowsky avatar Sep 27 '23 16:09 bazanowsky