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

[core] Fix running mocha related scripts on Windows locally #44664

Merged
merged 8 commits into from
Dec 16, 2024

Conversation

ChristopherJamesL
Copy link
Contributor

@ChristopherJamesL ChristopherJamesL commented Dec 5, 2024

I added documentation for the purpose of helping developers run unit tests if they are on a windows machine.

@mui-bot
Copy link

mui-bot commented Dec 5, 2024

Netlify deploy preview

https://deploy-preview-44664--material-ui.netlify.app/

Bundle size report

No bundle size changes (Toolpad)
No bundle size changes

Generated by 🚫 dangerJS against a5e98a3

@mj12albert mj12albert changed the title Update contributing documentation for Windows users to fix test:unit … [docs] Update instructions for running unit tests on Windows Dec 6, 2024
@mj12albert mj12albert added the docs Improvements or additions to the documentation label Dec 6, 2024
@mj12albert mj12albert requested a review from mnajdova December 6, 2024 06:20
@mj12albert
Copy link
Member

Hey thanks for contributing @ChristopherJamesL ~ I'm not really a windows user, just gotta wait for another reviewer to have a look

CONTRIBUTING.md Outdated
Comment on lines 203 to 204
If you encounter error: `'m)[jt]s?' is not recognized as an internal or external command` while running `pnpm test:unit`, it is likely because of an issue with how the test file extensions are handled in the script. Solution: Modify the `nx_test_unit` script in the root `package.json` to
`"nx_test_unit": "cross-env NODE_ENV=test mocha 'packages/**/*.test.{js,jsx,ts,tsx}' 'packages-internal/**/*.test.{js,jsx,ts,tsx}' 'docs/**/*.test.{js,jsx,ts,tsx}'"` This change ensures that the test files are correctly recognized with the appropriate extensions and allows running tests using the `--grep` flag on Windows.
Copy link
Member

@oliviertassinari oliviertassinari Dec 7, 2024

Choose a reason for hiding this comment

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

I haven't looked close, but at first sight, this can't fly, we can't possibly ask Windows contributors to change the source to run the tests, it's horrible DX.

Can we instead change the script source?

Either this was broken in #44143 or it's mochajs/mocha#5219. I can't easily test this on Windows.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yea I understand. I tried to look further into it to see if I could have a command that runs the tests successfully on both windows and mac but I wasn't able to find one.

Copy link
Member

@ZeeshanTamboli ZeeshanTamboli Dec 11, 2024

Choose a reason for hiding this comment

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

I am a Windows user, adding double quotes works, as shown in mochajs/mocha#5219. For example:

--- a/package.json
+++ b/package.json
@@ -95,7 +95,7 @@
     "nx_test_karma": "cross-env NODE_ENV=test karma start test/karma.conf.js",
     "nx_test_regressions_run": "mocha --config test/regressions/.mocharc.js --delay 'test/regressions/**/*.test.js'
",
     "nx_test_regressions_pigment_css_run": "mocha --config apps/pigment-css-vite-app/.mocharc.cjs --delay 'apps/pig
ment-css-vite-app/**/*.test.js'",
-    "nx_test_unit": "cross-env NODE_ENV=test mocha 'packages/**/*.test.?(c|m)[jt]s?(x)' 'packages-internal/**/*.tes
t.?(c|m)[jt]s?(x)' 'docs/**/*.test.?(c|m)[jt]s?(x)'"
+    "nx_test_unit": "cross-env NODE_ENV=test mocha \"packages/**/*.test.?(c|m)[jt]s?(x)\" \"packages-internal/**/*.
test.?(c|m)[jt]s?(x)\" \"docs/**/*.test.?(c|m)[jt]s?(x)\""
   },
   "dependencies": {
     "@googleapis/sheets": "^9.3.1",

@ChristopherJamesL Can you try the change and confirm? It needs to be applied to other scripts too.

@oliviertassinari oliviertassinari added test and removed docs Improvements or additions to the documentation labels Dec 7, 2024
@oliviertassinari oliviertassinari changed the title [docs] Update instructions for running unit tests on Windows [test] Update instructions for running unit tests on Windows Dec 7, 2024
@oliviertassinari oliviertassinari added the core Infrastructure work going on behind the scenes label Dec 7, 2024
@ZeeshanTamboli ZeeshanTamboli changed the title [test] Update instructions for running unit tests on Windows [core] Fix running mocha related scripts on Windows locally Dec 16, 2024
Copy link
Member

@ZeeshanTamboli ZeeshanTamboli left a comment

Choose a reason for hiding this comment

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

@ChristopherJamesL I updated the scripts. They work on Windows OS now.
@mj12albert Can you check if the changes work on Max OS X as well?

@mj12albert
Copy link
Member

mj12albert commented Dec 16, 2024

@mj12albert Can you check if the changes work on Max OS X as well?

Thanks @ZeeshanTamboli they do ~

I noticed there's also nx_test_regressions_run and nx_test_regressions_pigment_css_run that still have single quotes

@ZeeshanTamboli
Copy link
Member

ZeeshanTamboli commented Dec 16, 2024

I noticed there's also nx_test_regressions_run and nx_test_regressions_pigment_css_run that still have single quotes

@mj12albert They do work in single quotes on Windows OS as well. The glob pattern of asterisks (*) is recognized. Although not sure why not for other script glob patterns.

@ZeeshanTamboli ZeeshanTamboli merged commit 7193b1b into mui:master Dec 16, 2024
19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Infrastructure work going on behind the scenes test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants