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

Update dependencies and compatibility tests #1341

Merged
merged 7 commits into from
Nov 8, 2019
Merged

Conversation

vvanpo
Copy link
Contributor

@vvanpo vvanpo commented Nov 4, 2019

yarn install was failing with newer node versions (>=10) because the
version of fsevents was too old (no precompiled binaries), and node-gyp
would fail to build it.

yarn upgrade mitigates this issue, but a changes in prettier 1.18
(https://prettier.io/blog/2019/06/06/1.18.0.html) required yarn format
to also be run.

A missing dependency (rollup-plugin-json) was added to
@vue/test-utils, as it is imported in its build script.

`yarn install` was failing with newer node versions (>=10) because the
version of fsevents was too old (no precompiled binaries), and node-gyp
would fail to build it.

`yarn upgrade` mitigates this issue, but a change in `prettier` 1.18
(https://prettier.io/blog/2019/06/06/1.18.0.html) required `yarn format`
to also be run.

A missing dependency (`rollup-plugin-json`) was added to
`@vue/test-utils`, as it is imported in its build script.
@vvanpo
Copy link
Contributor Author

vvanpo commented Nov 4, 2019

As mentioned in the commit message, I had to add an explicit dependency within @vue/test-utils, as yarn build:test would fail due to this import:

const json = require('rollup-plugin-json')

What is unclear to me is why it wouldn't fail with the old lockfile. The package (rollup-plugin-json) was made available via @vue/server-test-utils and was therefore already in the lockfile. My yarn and node versions remained the same, and I reverted the lerna version just to check if it was that, but it still failed.

@vvanpo
Copy link
Contributor Author

vvanpo commented Nov 4, 2019

Hang on while I figure out the compat test failures.

@vvanpo
Copy link
Contributor Author

vvanpo commented Nov 4, 2019

So the compatibility tests are out-of-date, and there is duplication in multiple places. Running them during development using yarn test:compat runs the tests against versions of vue defined in scripts/test-compat-all.sh, but CircleCI attempts to reference its own, inconsistent values.

Further still, there are configuration inconsistencies in .circleci/config.yml: test_version_2.5.13 is actually attempting to install version 2.5.16, not 2.5.13.

The worst part, though, is that while CircleCI attempts to define its own versions to test against by calling yarn test:compat "2.5.16", for example, yarn test:compat does not take any arguments! This means every compatibility test runs for every definition, and if one fails, they all fail. Therefore, for the 6 test version definitions, the full test suite is run 36 separate times for every CI build.

@eddyerburgh is there any objection to calling scripts/test-compat.sh in scripts/test-compat-all.sh by using only the minor versions (i.e. installing the latest patch version for a given minor)? This would ensure compatibility tests always get run using the latest minor version, and would prevent the configurations from getting out-of-date. The only side-effect is that the compatibility tests are no longer deterministic (i.e. if vue patches a minor version, yarn test:compat will pull it down the next time it runs).

I would also like to call yarn test:compat once from CircleCI, and reverse the order of compatibility tests to run from newest-first. This would

  1. Provide consistency between running compatibility tests during development and during CI, preventing these configurations from ever getting out-of-sync.
  2. Run the most-likely-to-succeed tests first, ensuring the most useful error is output (since yarn test:compat uses set -e).
  3. Prevent the current scenario where all test cases are run redundantly 6 times over.

Update minor versions of running `yarn test:compat` to the latest patch
revisions.

Add optional argument to `yarn test:compat`, to allow running tests
against arbitrary `vue` versions.

Run compatibility tests in reverse order, to ensure the most relevant
test failures are displayed.

Run `yarn add` non-interactively, to force errors if `yarn test:compat`
is passed an invalid value.

Run `yarn add` without clobbering the lockfile.
@vvanpo
Copy link
Contributor Author

vvanpo commented Nov 4, 2019

OK, so since that update to yarn test:compat, CircleCI now correctly runs the compatibility tests and only 2.0.8 fails, as expected.

I'll have to figure out what exactly is causing the compatibility failure, perhaps a specific patch was fixed on all 2.x branches except for 2.0 for some reason. I'll look into it.

Having each compatibility test version specified in both the CI config
and the package script leads to inconsistencies over time. The CI config
can just use the package script.
Common build dependencies across sub-packages can get out-of-sync over
time. Currently, `rollup` and plugins is used across both `test-utils`
and `server-test-utils`, but different versions are specified. This
requires duplicate installations, and prevents hoisting.

Since `devDependencies` are not present in published packages, and since
the monorepo is used exclusively for building the subpackages, it makes
sense to define all build dependencies in the root package.

Move all `devDependencies` in the root package to `dependencies`, to
avoid unmet peer dependency errors in `yarn`:
yarnpkg/yarn#5810

Update each of these dependencies to their latest version, in particular
`rollup`, which has since released stable versions.

See https://github.com/lerna/lerna/blob/master/doc/hoist.md#lerna-hoisting
@vvanpo
Copy link
Contributor Author

vvanpo commented Nov 5, 2019

I think I've got to the bottom of the rollup-plugin-json issue. The package was upgraded from 2.3.0 -> 2.3.1, and that version now includes an explicit peer dependency on rollup. However, yarn does not consider peer dependencies in workspaces to be met by dev dependencies in the root (yarnpkg/yarn#5810), and I assume that the resulting error caused yarn to un-hoist the package. With 2.3.0, there was no unmet peer dependency, so the plugin was hoisted to the root package and therefore all sub-packages could resolve it.

I've done two things to address this problem:

  1. I've moved all dev dependencies to the root package. This ensures they are not duplicated, prevents them from getting out of sync, and explicitly hoists them (thereby avoiding silent hoisting failures from yarn).
  2. Moved all root package dependencies to regular dependencies, instead of devDependencies. This prevents the unmet peer dependency problem during yarn add and upgrade. Because the root package is private, and not published, there is no difference for clients of its sub-packages.

I'm still unsure of what is causing all the test failures in the last compatibility test (Vue version 2.0.8). I tried stepping through every dependency using yarn upgrade -P to try to isolate the culprit, but only the full upgrade caused failures. I'll keep digging.

@vvanpo vvanpo changed the title Update lockfile Update dependencies and compatibility tests Nov 6, 2019
Each compatibility test installs a legacy version of `vue`, and builds
each package against this version before running the test suites. This
is assumedly in error, as the only build that gets published is the one
built against the dependencies in the lockfile.

To properly test compatibility for consumers using older versions of
`vue` to satisfy each package's peer dependency, we need to test against
the build that would actually get published. The fix is to run `yarn
build:test` only once, before any compatibility tests are run.
The upgrade from `rollup` beta -> stable went without breakages, so
clearly we don't need to overly constrain updated dependencies using the
`^` semver specifier.
The updates from 1d6950a have to be partially reverted to allow for the
last compatiblity test suite to pass: `vue@2.0.8`. There is some unclear
combination of dependencies that causes 80 unit test failures for only
the 2.0 version of vue.
@vvanpo
Copy link
Contributor Author

vvanpo commented Nov 6, 2019

Unfortunately I was not able to get to the bottom of the failing compatibility tests, I've spent too long on it already. I don't fully understand all the pieces yet, when I do maybe I'll revisit it. I've opted to just revert most of the locked dependencies.

The build system improvements and dependency simplification/hoisting is still valuable, though.

@vvanpo
Copy link
Contributor Author

vvanpo commented Nov 8, 2019

@gkatsanos what's the merge procedure for pull requests in this project? How many reviewers are needed, and who is in charge of merging?
The contributors/maintainers fields are absent from the package.json, so I'm unsure of who I can ask for reviews.

Copy link
Member

@kazupon kazupon left a comment

Choose a reason for hiding this comment

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

@vvanpo
Thank you very much!
LGTM!

@kazupon kazupon merged commit 5c61b29 into vuejs:dev Nov 8, 2019
@vue-bot
Copy link

vue-bot commented Nov 8, 2019

Hey @vvanpo, thank you for your time and effort spent on this PR, contributions like yours help make Vue better for everyone. Cheers! 💚

@vvanpo vvanpo deleted the update-deps branch November 8, 2019 22:17
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