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

fix(test): test:client silently failing on Travis #3343

Merged
merged 2 commits into from
Jul 26, 2019

Conversation

devoto13
Copy link
Collaborator

It is not enough to check exit code, because when process crashes code is set to null and err is passed instead. Adjusted build script accordingly and forced new version of natives to make tests pass.

Example of failing tests, which didn't fail build:
https://travis-ci.org/karma-runner/karma/jobs/537027667#L1046

@devoto13 devoto13 changed the title Fix test:client silently failing on Travis fix(test): test:client silently failing on Travis Jul 25, 2019
Copy link
Contributor

@johnjbarton johnjbarton left a comment

Choose a reason for hiding this comment

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

The top of this file says

THIS IS AN AUTOGENERATED FILE. DO NOT EDIT THIS FILE DIRECTLY.

We can't have the core files blocked by downstream issues. So what change in this project would update natives?

@devoto13
Copy link
Collaborator Author

devoto13 commented Jul 25, 2019

The warning is to prevent people from breaking lock file by incorrect edits. The change does exactly what it is supposed to: makes Yarn resolve natives@^1.1.0 to 1.1.6 instead of 1.1.1.

In fact this change was done by removing natives record from the yarn.lock and then running yarn install to force Yarn to resolve it again. There is no other way to force Yarn re-resolve only specific package as far as I know.

@johnjbarton
Copy link
Contributor

Ok but this file is a product of some other config which needs to be fixed or we'll just overwrite it.

And we don't even use yarn AFAIK, so I don't understand how this is relevant, except to someone who uses yarn install I suppose.

@devoto13
Copy link
Collaborator Author

devoto13 commented Jul 25, 2019

Ok but this file is a product of some other config which needs to be fixed or we'll just overwrite it.

This is not completely true. While yarn.lock is produced by running yarn install from the package.json it won't be overwritten on next install. The idea of the lock file is to capture version resolutions for the dependency tree and re-use this resolutions for subsequent installs to support reproducible builds. If we look at natives dependency it is specified as natives@^1.1.0, which may install 1.1.1, 1.1.2, 1.1.3 or whatever is the latest version at the time of install. At the time, when it was added it was resolved to 1.1.1 and this is captured by the yarn.lock file. When a developer uses yarn to install dependencies they will always get version 1.1.1 and therefore are protected from potential breaking change in the newer release of this package.

See https://yarnpkg.com/en/docs/yarn-lock#toc-check-into-source-control for more information:

All yarn.lock files should be checked into source control (e.g. git or mercurial). This allows Yarn to install the same exact dependency tree across all machines, whether it be your coworker’s laptop or a CI server.


And we don't even use yarn AFAIK, so I don't understand how this is relevant, except to someone who uses yarn install I suppose.

Yarn is used on Travis builds (https://docs.travis-ci.com/user/languages/javascript-with-nodejs#using-yarn) and I believe some developers use it for local development. It is not used and does not affect end users of the Karma. If you think that this repo should stop using Yarn completely then we should remove yarn.lock and potentially introduce package-lock.json (alternative from NPM) to provide the same functionality as yarn.lock does.

@johnjbarton
Copy link
Contributor

If our tests must be run against natives 1.1.6, then shouldn't we add a dependency on natives ^1.1.6 to package.json? Then the yarn.lock will work on travis and won't slide back if we run yarn install.

@devoto13
Copy link
Collaborator Author

It comes from the following dependencies chain: karma-browserstack-launcher#browserstacktunnel-wrapper#unzip#fstream#graceful-fs#natives, so there is no package.json to update in this repo. yarn.lock will not slide back to older version on yarn install, since Yarn will not change locked versions unless somebody takes targeted effort to make it do so (like I did in this PR by changing yarn.lock).

@johnjbarton
Copy link
Contributor

The idea of the lock file is to capture version resolutions for the dependency tree and re-use this resolutions for subsequent installs to support reproducible builds.

But this makes yarn.lock an output, a build result. It's the result of version resolution, not the definition of versions. If I delete yarn.lock and redo the version resolution I'll get a valid yarn.lock a lot of changes. Will I notice that natives slid back to 1.1.1?

Is there a reason not to dep natives 1.1.6 in the package.json with a comment that we want to fix the downstream chain?

I gather that graceful-fs only needs natives ^1.1.1, except on node 10 where it needs ^1.1.3. So its the broken package.

@johnjbarton
Copy link
Contributor

Based on your analysis I create PR #3344, do you think it could achieve your goal?

@devoto13
Copy link
Collaborator Author

Yes, this solution is much better!
Didn't go with it, because of browserstack/browserstack-local-nodejs#77, which was apparently fixed two days ago.
Please merge your PR first and I'll update this one to only include fix for the build script.

Apparently it is not enough to check `code`, because when process
crashes `code` is set to `null` and `err` is passed instead.

Example of failing tests, which didn't fail build:
https://travis-ci.org/karma-runner/karma/jobs/537027667#L1046
@johnjbarton
Copy link
Contributor

Thanks!

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.

2 participants