dperini / nwsapi

Fast CSS Selectors API Engine
MIT License
103 stars 35 forks source link

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

Open sagar1111212121 opened 1 year ago

sagar1111212121 commented 1 year ago

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?

dperini commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago

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

sagar1111212121 commented 1 year ago

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 commented 1 year ago

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

"devDependencies": {
  "nwsapi": "2.2.5"
},
"overrides": {
  "nwsapi": "$nwsapi"
}
dperini commented 1 year ago

@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 commented 1 year ago

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

nandeshwarshubh commented 1 year ago

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

dperini commented 1 year ago

@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 commented 1 year ago

For anyone using yarn, this workaround worked for me:

"resolutions": {
  "**/nwsapi": "2.2.5"
}
val1984 commented 1 year ago

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

{
  "pnpm": {
    "overrides": {
      "nwsapi": "2.2.5"
    }
  }
}
isc30 commented 1 year ago

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

dep commented 1 year ago

@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 commented 1 year ago

@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 commented 1 year ago

@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 commented 1 year ago

@dperini Thanks for the quick action

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

sagar1111212121 commented 1 year ago

@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 commented 1 year ago

@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 commented 1 year ago

@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 commented 1 year ago

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

dperini commented 1 year ago

@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 commented 1 year ago

@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 commented 1 year ago

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

Xe11o commented 1 year ago

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 commented 1 year ago

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

bazanowsky commented 11 months ago

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?