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

feat: add shortcut for running karma tests #3750

Merged

Conversation

AllanOricil
Copy link
Contributor

@AllanOricil AllanOricil commented Sep 28, 2023

Details

enables developers to run tests from @lwc/integration-karma using yarn test:karma from the root dir.

Does this pull request introduce a breaking change?

  • ✅ No, it does not introduce a breaking change.

Does this pull request introduce an observable change?

  • ✅ No, it does not introduce an observable change.

GUS work item

N/A

@AllanOricil AllanOricil requested a review from a team as a code owner September 28, 2023 19:50
Copy link
Collaborator

@nolanlawson nolanlawson 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 PR, but I'm not really a big fan of this approach. The Jest/Karma tests take up to a minute to run (depending on how fast your machine is), and IME that really slows down velocity during development.

On top of that, if you really want to know whether your commit will pass CI, there are about a dozen different modes it has to run in (see the circleci config), plus the WebDriverIO tests, tests against various browsers, ad-hoc scripts that check for things like license headers... running just Jest/Karma is a false sense of security.

Right now lint-staged does the bare minimum of making sure everything is ESLint formatted and Prettier formatted. I think that's enough for the time being. I've worked on projects where it runs all the unit tests on every commit, and I find that I type -n every time, which nullifies the value of the check.

@@ -20,6 +20,7 @@
"test": "jest --config ./scripts/jest/root.config.js",
"test:debug": "node --inspect node_modules/.bin/jest --config ./scripts/jest/root.config.js --runInBand --watch",
"test:ci": "yarn test --no-cache --coverage --runInBand",
"test:karma": "nx test @lwc/integration-karma",
Copy link
Collaborator

Choose a reason for hiding this comment

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

On its own, this is a good change... it's a nice shorthand, and we were being inconsistent before.

@AllanOricil
Copy link
Contributor Author

most difficult PR of my life 🥹

@nolanlawson nolanlawson mentioned this pull request Sep 28, 2023
@abdulsattar abdulsattar changed the title feat: ensure tests run before commits feat: add shortcut for running karma tests Sep 28, 2023
@AllanOricil
Copy link
Contributor Author

Non related. I just want to vent out.

I'm really relieved that I started working after the jquery era. Look at this app

https://github.com/node-red/node-red/blob/master/packages/node_modules/%40node-red/editor-client/src/js/red.js

😐

@nolanlawson
Copy link
Collaborator

Thank you! ❤️

@nolanlawson nolanlawson merged commit 0c1de0a into salesforce:master Sep 29, 2023
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants