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

SALTO-1155: Update jest version #1804

Merged
merged 5 commits into from
Jan 31, 2021
Merged

Conversation

ori-moisis
Copy link
Contributor

@ori-moisis ori-moisis commented Jan 27, 2021

  • Update jest and jest types to newer version
  • Addresses this security advisory
  • Minor fixes to tests that were exposed by the more strict version of jest

  • Probably easier to review each commit on its own
  • The new jest version found a couple of real bugs in our tests and code, those were fixed in previous PRs

Release Notes:
None

@ori-moisis ori-moisis changed the title Update jest version SALTO-1155: Update jest version Jan 27, 2021
@ori-moisis
Copy link
Contributor Author

build fails because of a yarn bug that was only fixed in yarn v2 (link)
I really hope to find a workaround...

@coveralls
Copy link

coveralls commented Jan 27, 2021

Coverage Status

Coverage remained the same at 94.184% when pulling 194f6fd on ori-moisis:update/jest into 34e8dde on salto-io:master.

noamhammer
noamhammer previously approved these changes Jan 27, 2021
@ori-moisis ori-moisis dismissed noamhammer’s stale review January 27, 2021 15:41

Not done upgrading packages, the old jest version is still in the yarn.lock file

@ori-moisis ori-moisis marked this pull request as draft January 27, 2021 15:42
@ori-moisis ori-moisis force-pushed the update/jest branch 2 times, most recently from f403cd4 to 785ff73 Compare January 27, 2021 17:15
@ori-moisis ori-moisis marked this pull request as ready for review January 27, 2021 17:25
@ori-moisis ori-moisis requested a review from a team January 27, 2021 17:25
- run: yarn
# Yarn has some random race-condition failures, trying multiple times seems to help
# when the source of the problem is yarn itself and not the packages
- run: yarn || yarn || yarn
Copy link
Contributor

Choose a reason for hiding this comment

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

will we always need this, or only until everything is upgraded?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is probably only relevant whenever we change any dependency (otherwise the first "yarn" would just do nothing an succeed, so the other two won't run)
I think we should leave it in though because I wouldn't want to re-discover this yarn bug every time we upgrade a package

@@ -19,10 +19,18 @@ const deepMerge = require('../../build_utils/deep_merge')
module.exports = deepMerge(
require('../../eslintrc.js'),
{
overrides: [
Copy link
Contributor

Choose a reason for hiding this comment

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

just out of curiosity, why was this change needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

because of the dynalite config file, because it is a .js file eslint wanted to lint it.
in the base eslint config we set a project for all files (that is probably a mistake, but I did not want to deal with that as part of this PR) but because this config .js file is not really a part of the project it made eslint fail.
This configuration override makes it so only .ts files will have the project attribute set.

btw - The project attribute is require only for lint rules that rely on types, I think we don't have any such rules enabled so it might be possible to get rid of this altogether, but again, didn't want to deal with that as part of this PR

@ori-moisis ori-moisis merged commit 2ac7d5e into salto-io:master Jan 31, 2021
@ori-moisis ori-moisis deleted the update/jest branch January 31, 2021 11:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants