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

Roll back to previously locked versions of packages that are causing test failures #1083

Merged
merged 3 commits into from Jan 7, 2020

Conversation

@mturley
Copy link
Contributor

mturley commented Jan 6, 2020

When @mzazrivec mentioned the CI failures in #1082 (comment) I noticed many of them were unrelated to the code changes in that PR. I realized that since we removed yarn.lock in #1079 and we use the ^ version approximation syntax in our package.json, we allowed some packages to be upgraded on a fresh install. I verified this by checking out the master branch, removing my local yarn.lock, re-running yarn install and then running yarn test, which failed.

The failures were related to:

  • changes to lint rules due to updated versions of prettier and eslint-plugin-react
  • unexpected new props in snapshots due to an updated version of react-ellipsis-with-tooltip
  • errors in certain unit tests which create redux stores without all the reducers matching the properties passed into the store, which is a check introduced in an updated version of redux

This PR reverts those packages to the specific versions that were locked in the yarn.lock file we removed, by removing the ^ and just using version numbers alone. The tests pass now.

We should possibly eventually upgrade these packages and make the code changes necessary to make the tests pass with the new versions, but I didn't think our CI should fail just because something outside the repo changed. We can make those changes when we're ready to.

I'm also trusting (hoping) that all the other packages we are now pulling in newer versions of are honoring their semantic versioning and not releasing any breaking changes that we'll get with this package.json, and that this kind of thing will only break unit tests that assume the lack of new features. I really hope I'm right about that.. maybe we should be avoiding the ^ syntax entirely.

This will unblock all other open PRs, which should be rebased on these changes.

@mturley mturley requested review from Fryguy, himdel and mzazrivec Jan 6, 2020
@mturley mturley added this to In progress in v2v UI via automation Jan 6, 2020
@mturley mturley added the dependencies label Jan 6, 2020
@miq-bot

This comment has been minimized.

Copy link
Member

miq-bot commented Jan 6, 2020

Checked commits mturley/manageiq-v2v@d6370b7~...150e9af with ruby 2.5.5, rubocop 0.69.0, haml-lint 0.20.0, and yamllint 1.10.0
0 files checked, 0 offenses detected
Everything looks fine. 👍

@mzazrivec mzazrivec self-assigned this Jan 7, 2020
@mzazrivec

This comment has been minimized.

Copy link
Contributor

mzazrivec commented Jan 7, 2020

I'm confirming that the specific package versions now make yarn prettier and yarn test pass.

Merging.

@mzazrivec mzazrivec merged commit ff9544b into ManageIQ:master Jan 7, 2020
3 checks passed
3 checks passed
Hakiri No security warnings were found.
Details
codeclimate All good!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
v2v UI automation moved this from In progress to Done Jan 7, 2020
@himdel

This comment has been minimized.

Copy link
Contributor

himdel commented Jan 7, 2020

👍 I commented in #1079 (comment),
but I think the problem is that ^ is simply too optimistic and ~ usually does the right thing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
v2v UI
  
Done
4 participants
You can’t perform that action at this time.