Pkg-tests testsuite #5392
Pkg-tests testsuite #5392
Conversation
arcanis
force-pushed
the
pkg-tests
branch
3 times, most recently
from
f4da332
to
7161784
Compare
pkg-tests/.eslintrc.json
Outdated
| "flowtype/require-valid-file-annotation": "off", | ||
| "flowtype/require-return-type": "off" | ||
| } | ||
| } |
There was a problem hiding this comment.
Disabled flow (in this directory only) for the time being, the utils don't really need typing right now (especially since they're by definition covered by the tests :).
There was a problem hiding this comment.
You are probably right but I think we should generally try to use flow types across the board. It seems that you could add flow types to everything rather quickly 1�7 do you mind spending some time on this? It will make it significantly easier for other people to work on this code, for example in Nuclide, where we have type definitions available and auto-completion.
There was a problem hiding this comment.
Also helps me review the code ;)
| // eslint-disable-next-line | ||
| `require('dep-loop-entry') === require('dep-loop-entry').dependencies['dep-loop-exit'].dependencies['dep-loop-entry']`, | ||
| ), | ||
| ); |
There was a problem hiding this comment.
what do you expect here? Missing comparison! If you use eslint-plugin-jest, this will error.
There was a problem hiding this comment.
Good catch! I ran eslint from the repo root, and errors were successfully reported, but not this one. Maybe because of the await?
|
Really sweet. Can you fix CI, add flow types and possibly add this to the main Jest run with the multi-project-runner feature? https://facebook.github.io/jest/blog/2017/05/06/jest-20-delightful-testing-multi-project-runner.html |
|
This change will decrease the build size from 10.49 MB to 10.46 MB, a decrease of 28.18 KB (0%)
|
|
Fixed Flow but prevented it from running on pkg-tests for now on CI environments. Integrating pkg-tests to our testsuite will be somewhat complex (we need to install its own dependencies + build yarn itself, and we can't make it a workspace because the root package.json is not private), so I'd like to do this in another PR to make things easier to review. You can still run Flow if you go into |
|
My 1st thought: "It's a workspace nested in a workspace?" Could you add a readme file that explains how this works and how to add tests? I think that was one of the shortcomings of the unit test framework; figuring out how to add tests was a bit of a painful expedition. From a quick glance, it looks like this fires up an http server that acts as a custom registry that then serves the fixtures packages? That's a cool idea, and probably a lot easier to manage than what we do in the unit tests with mocking http requests. I assume it is not a replacement for the existing unit tests, but a new suite of high-level "end to end" tests? I'm also curious if you've showed this to anyone on npm team (zkat maybe?) to see if this is something they would like to use as well. It'd be really damn cool if we had a shared suite of "smoke tests". Will this be able to handle things like github urls? Or really, all the crazy url formats that npm has introduced over the years? i.e. if I make a package in fixtures that has a dependency on Is this just testing the The tests all fail for me with the error: How does it resolve what package manager to run? It looks like it's expecting yarn 1.4.1 from somewhere? Edit: If I change the path in |
|
Good questions!
Yarn doesn't support nested workspaces. However it's not one, because the pkg-tests directory is not configured as a workspace of the yarn repository (I think on the long term it might be valuable to move yarn itself into the
Will do
Yep exactly!
I think provided we can implement a decent number of tests it might prove a worthy replacement for most of our install tests (such as
Not yet, I wanted to make it work here before bothering them, but I'd really like to know what they think of this once we get something going. I think the best thing in this project is that it's mostly project-agnostic, so if everyone contributes we can end up with something valuable for everyone.
Git (and exotic registries in general) will be quite more difficult because we'll need to run a light git server somehow. I'm sure it can be done tho! Maybe by introducing settings in our package manager to specify a different target for github aliases (like resolving "foo/bar" to "localhost/foo/bar#semver" instead of "github.com/foo/bar#semver").
Right now yes, I think work should focus around here since it's the most common and important brick, and all other commands are kinda optional (
It uses a driver file which specifies what should be done when the tests invoke the I'll have to investigate the timeouts - can you try to increase the |
packages/pkg-tests/yarn.test.js
Outdated
| const pkgDriver = generatePkgDriver({ | ||
| runDriver: (path, args, {registryUrl}) => { | ||
| const extraArgs = [`--cache-folder`, `${path}/.cache`]; | ||
| return execFile(process.execPath, [`${process.cwd()}/../artifacts/yarn-1.4.1.js`, ...extraArgs, ...args], { |
There was a problem hiding this comment.
Path is no longer valid since I moved pkg-tests into packages/pkg-tests, I need to update it
There was a problem hiding this comment.
The version changes constantly too. Too lazy to directly submit a PR to your branch... I wrote this to auto detect yarn:
// @flow
const fs = require('fs');
const {
tests: {generatePkgDriver, startPackageServer, getPackageRegistry},
exec: {execFile},
} = require(`pkg-tests-core`);
const {basic: basicSpecs, dragon: dragonSpecs} = require(`pkg-tests-specs`);
const yarnFileMatcher = /^yarn-\d+\.\d+\.\d+\.js$/;
const getYarnPath = function(): string {
const artifactsFolder = `${process.cwd()}/../../artifacts`;
const yarnFile = fs.readdirSync(artifactsFolder).find(file => yarnFileMatcher.test(file));
if (!yarnFile) {
throw new Error(`Unable to locate yarn in ${artifactsFolder}`);
}
return `${artifactsFolder}/${yarnFile}`;
};
const pkgDriver = generatePkgDriver({
runDriver: (path, args, {registryUrl}) => {
const extraArgs = [`--cache-folder`, `${path}/.cache`];
const yarnPath = getYarnPath();
return execFile(process.execPath, [yarnPath, ...extraArgs, ...args], {
env: {[`NPM_CONFIG_REGISTRY`]: registryUrl, [`YARN_SILENT`]: `1`},
cwd: path,
});
},
});
beforeEach(async () => {
await startPackageServer();
await getPackageRegistry();
});
basicSpecs(pkgDriver);
dragonSpecs(pkgDriver);
|
A long timeout seems to expose an ESOCKETTIMEDOUT error? |
|
Fixed and fixed |
package.json
Outdated
| @@ -107,7 +107,7 @@ | |||
| "build-dist": "bash ./scripts/build-dist.sh", | |||
| "build-win-installer": "scripts\\build-windows-installer.bat", | |||
| "dupe-check": "yarn jsinspect ./src", | |||
| "lint": "eslint . && flow check", | |||
| "lint": "eslint src __tests__ && flow check", | |||
There was a problem hiding this comment.
It was for when there was no flow types, I'll revert it
packages/pkg-tests/.flowconfig
Outdated
| @@ -0,0 +1,18 @@ | |||
| [ignore] | |||
There was a problem hiding this comment.
Why do you need a separate flowconfig here?
|
|
||
| export type {PackageDriver} from './utils/tests'; | ||
|
|
||
| const exec = require(`./utils/exec`); |
There was a problem hiding this comment.
This isn't the code style we are using for requires. Can you use single quotes?
| @@ -17,6 +17,7 @@ const path = require('path'); | |||
|
|
|||
| const installFixturesLoc = path.join(__dirname, '..', 'fixtures', 'install'); | |||
|
|
|||
| // $FlowFixMe I don't understand the error | |||
There was a problem hiding this comment.
me neither. cc @calebmer any idea what this is supposed to be?
packages/pkg-tests/.eslintrc.json
Outdated
| @@ -0,0 +1,4 @@ | |||
| { | |||
There was a problem hiding this comment.
Also not sure about this one. Can we please re-use the lint rules from the main folder? I don't think this test framework/suite should adhere to different lint rules than others.
| @@ -0,0 +1,58 @@ | |||
| // @flow | |||
There was a problem hiding this comment.
The rest of Yarn uses /* @flow */. Can we do the same here?
There was a problem hiding this comment.
I'm gonna stamp this but I think it would be better next time to split a diff like this up into ten smaller ones, something like this:
- Upgrade flow to 0.66.
- Fix timers/update flow type to use TimeoutID.
- Add scaffold for new test infra.
- Add lots of test fixtures.
- One commit for each new test you are adding.
Please consider splitting it up next time to reduce the review burden on people who look at your PRs. Thanks.
Summary
This PR adds a new experimental testsuite. It has a few important differences compared to our current one:
Installclass (it spawns a process instead)no-deps... has no deps. I let you guess forone-fixed-dep).In this end, I think this experiment should benefit both the community at large and us in the long term. Having a generic testsuite will make it much easier for everyone involved to go fast without breaking things.
Test plan
Run
yarn && yarn jest yarn.test.jsin thepkg-testsdirectory.