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

Report test summaries to CircleCI #4704

Merged
merged 7 commits into from Jul 7, 2018
Merged

Conversation

j-f1
Copy link
Member

@j-f1 j-f1 commented Jun 16, 2018

Fixes #4510.

@j-f1
Copy link
Member Author

j-f1 commented Jun 16, 2018

screen shot 2018-06-16 at 18 48 06

(it uses default function parameters)
@@ -20,7 +20,12 @@ shell.exec("npm init -y", { cwd: tmpDir });
shell.exec(`npm install "${tarPath}"`, { cwd: tmpDir });
shell.config.silent = false;

const cmd = `yarn test --color --runInBand --reporters default --reporters jest-junit ${
const reporters =
+process.version.match(/^v\d+\./)[1] >= 6
Copy link
Member

Choose a reason for hiding this comment

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

As far as I can tell this condition is always false. I think you forgot parentheses around \d+:

+process.version.match(/^v(\d+)\./)[1] >= 6

Copy link
Member

Choose a reason for hiding this comment

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

Or require('semver').major(process.version) >= 6

? "--reporters default --reporters jest-junit"
: "";

const cmd = `yarn test --color --runInBand ${reporters} ${
Copy link
Member

Choose a reason for hiding this comment

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

Why not add another conditional?

${process.env.JEST_JUNIT ? "--reporters ..." : ""}

And then just set this env in test_prod_node9

package.json Outdated
@@ -104,5 +105,8 @@
"build": "node ./scripts/build/build.js",
"build-docs": "node ./scripts/build-docs.js",
"check-deps": "node ./scripts/check-deps.js"
},
"jest-junit": {
Copy link
Member

Choose a reason for hiding this comment

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

Is it possible to move this to jest.config.js?

Copy link
Member Author

Choose a reason for hiding this comment

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

nope. Is there a way to remove it before publishing?

Copy link
Member

@duailibe duailibe Jul 1, 2018

Choose a reason for hiding this comment

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

Yeah we already modify it in the build script:

async function preparePackage() {
const pkg = await util.readJson("package.json");
pkg.bin = "./bin-prettier.js";
pkg.engines.node = ">=4";
delete pkg.dependencies;
delete pkg.devDependencies;
pkg.scripts = {
prepublishOnly:
"node -e \"assert.equal(require('.').version, require('..').version)\""
};
pkg.files = ["*.js"];
await util.writeJson("dist/package.json", pkg);
await util.copyFile("./README.md", "./dist/README.md");
}

Copy link
Contributor

Choose a reason for hiding this comment

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

@j-f1 If I'm reading the docs right, the junit config could be put into the "reporters" part of jest.config.js (although Prettier doesn't use that field currently, so it's probably simpler to just remove it as part of the build script.)

Copy link
Member Author

@j-f1 j-f1 Jul 3, 2018

Choose a reason for hiding this comment

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

After re-reading the docs, it looks like package.json and environment variables are the only way to configure the package without explicitly enabling it. Putting the config in jest.config.js would cause the package to always be loaded, which would cause issues on older Node versions.

Copy link
Contributor

Choose a reason for hiding this comment

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

@palmerj3 is that correct? If so, we should fix it

Copy link
Member Author

Choose a reason for hiding this comment

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

@SimenB I meant that if I put the config in the reporters section, that would enable the reporter, which I only want to do on the one test run.

Copy link

Choose a reason for hiding this comment

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

That's correct. You can have configuration live in jest.config.js if you want and use the reporter configuration. Most folks have jest-junit loaded as a reporter all the time and just gitignore the junit.xml file since it doesn't add very much overhead at all.

But if you REALLY don't want a junit.xml file generated locally then perhaps use the CI=true environment variable and update your jest.config.js to have a conditional for that.

Copy link
Contributor

Choose a reason for hiding this comment

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

reporters: [
  "default",
  process.env.CI ? ["jest-junit", { ...config }] : null,
].filter(Boolean);

or something like that should work, shouldn't it?

Copy link

Choose a reason for hiding this comment

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

Yep, exactly.

@j-f1 j-f1 merged commit dc362c2 into prettier:master Jul 7, 2018
@j-f1 j-f1 deleted the test-summaries branch July 7, 2018 16:11
@@ -80,6 +80,7 @@ async function preparePackage() {
pkg.engines.node = ">=4";
delete pkg.dependencies;
delete pkg.devDependencies;
delete pkg["jest-junit"];
Copy link
Contributor

Choose a reason for hiding this comment

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

this isn't necessary, is it?

Copy link
Member Author

Choose a reason for hiding this comment

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

@lock lock bot added the locked-due-to-inactivity Please open a new issue and fill out the template instead of commenting. label Oct 15, 2018
@lock lock bot locked as resolved and limited conversation to collaborators Oct 15, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
locked-due-to-inactivity Please open a new issue and fill out the template instead of commenting.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants