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

cleanup(js): fix test cases on windows #27300

Merged

Conversation

robertIsaac
Copy link
Contributor

@robertIsaac robertIsaac commented Aug 5, 2024

Current Behavior

running nx test js fail on windows

Expected Behavior

running nx test js succeed

side notes

  1. I was skipping packages/js/src/plugins/typescript/plugin.spec.ts because it makes all tests fail, and even in wsl it's very flaky, it fail 90% of the times, but I couldn't figure out why
    I think it's worth looking at from someone with more experience with the repo
  2. for some of the cases that I fixed I'm not sure if I should change the code to always return / or should change the test to adapt / in linux and \ in windows, so please if I mistaken one of them let me know and I will do it the other way around but I believe it should be fine since in windows foo/bar does work as a path still

@robertIsaac robertIsaac requested a review from a team as a code owner August 5, 2024 22:02
Copy link

vercel bot commented Aug 5, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Skipped Deployment
Name Status Preview Updated (UTC)
nx-dev ⬜️ Ignored (Inspect) Visit Preview Aug 14, 2024 3:53pm

@leosvelperez
Copy link
Member

@robertIsaac thanks for sending these fixes! Could you please rebase the PR? I believe some of the fixes here might have already landed on master as part of other changes.

@robertIsaac robertIsaac force-pushed the bugfix/fix-js-package-on-windows branch from fb462fa to d3fa62f Compare August 12, 2024 19:52
@robertIsaac
Copy link
Contributor Author

@leosvelperez done

@robertIsaac robertIsaac force-pushed the bugfix/fix-js-package-on-windows branch 3 times, most recently from 5f0ce3a to 91ad3f0 Compare August 14, 2024 15:31
@robertIsaac robertIsaac force-pushed the bugfix/fix-js-package-on-windows branch from 91ad3f0 to 6a98430 Compare August 14, 2024 15:43
@robertIsaac
Copy link
Contributor Author

robertIsaac commented Aug 14, 2024

and @leosvelperez
about packages/js/src/plugins/typescript/plugin.spec.ts test
do you have any idea how to fix

  ● Plugin: @nx/js/typescript › should create nodes for root tsconfig.json files                                                                                                                                                 
                                                                                                                                                                                                                                 
    EBUSY: resource busy or locked, rmdir 'C:\Users\rober\AppData\Local\Temp\typescript-plugin5yXujB'

      82 |
      83 |   cleanup() {
    > 84 |     rmSync(this.tempDir, { recursive: true });
         |           ^
      85 |     setWorkspaceRoot(this.previousWorkspaceRoot);
      86 |   }
      87 |

      at TempFs.cleanup (../nx/src/internal-testing-utils/temp-fs.ts:84:11)
      at Object.<anonymous> (src/plugins/typescript/plugin.spec.ts:32:12)

and if this file is enabled, almost all of the tests fail

with it being ignored (using xdescribe)

Test Suites: 1 skipped, 29 passed, 29 of 30 total
Tests:       33 skipped, 266 passed, 299 total
Snapshots:   229 passed, 229 total
Time:        15.468 s
Ran all test suites.

with it being active

Test Suites: 27 failed, 3 passed, 30 total
Tests:       31 failed, 17 passed, 48 total
Snapshots:   15 failed, 53 passed, 68 total
Time:        3.904 s, estimated 20 s

@leosvelperez
Copy link
Member

and @leosvelperez
about packages/js/src/plugins/typescript/plugin.spec.ts test
do you have any idea how to fix

I'm unsure about that one. Let's merge this and I'll try to set some time next week to check it.

@leosvelperez leosvelperez changed the title fix(js): tests on windows cleanup(js): fix test cases on windows Aug 16, 2024
Copy link
Member

@leosvelperez leosvelperez left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution!

@leosvelperez leosvelperez merged commit 69c989e into nrwl:master Aug 16, 2024
6 checks passed
@robertIsaac robertIsaac deleted the bugfix/fix-js-package-on-windows branch August 16, 2024 17:44
FrozenPandaz pushed a commit that referenced this pull request Aug 19, 2024
## Current Behavior
running `nx test js` fail on windows

## Expected Behavior
running `nx test js` succeed

## side notes
1. I was skipping `packages/js/src/plugins/typescript/plugin.spec.ts`
because it makes all tests fail, and even in wsl it's very flaky, it
fail 90% of the times, but I couldn't figure out why
I think it's worth looking at from someone with more experience with the
repo
2. for some of the cases that I fixed I'm not sure if I should change
the code to always return `/` or should change the test to adapt `/` in
linux and `\` in windows, so please if I mistaken one of them let me
know and I will do it the other way around but I believe it should be
fine since in windows `foo/bar` does work as a path still

(cherry picked from commit 69c989e)
Copy link

This pull request has already been merged/closed. If you experience issues related to these changes, please open a new issue referencing this pull request.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 24, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants