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

Generate unique xml file names #108

Merged
merged 3 commits into from
Oct 22, 2019
Merged

Generate unique xml file names #108

merged 3 commits into from
Oct 22, 2019

Conversation

andrcuns
Copy link
Contributor

@andrcuns andrcuns commented Oct 22, 2019

Feature proposal

There might be several cases when having a static xml name is undesirable, for example if single CI job runs multiple test suites as multiple commands, reports get overwritten. Another use case is knapsack-pro which splits test execution in batches. This seems to solve issue describe in #106
This PR does following things:

  • Generate unique report file names by default if no explicit option is provided
  • Update dependencies so project can be built on node v12, remove old unsupported node versions from tests

@palmerj3
Copy link
Collaborator

@andrcuns thanks for the PR!

The second change looks good but I cannot have jest-junit generate a unique xml by default. The vast majority of folks using this rely on junit.xml being generated and a lot of CI systems rely on this file being there for integration.

There is currently the JEST_JUNIT_OUTPUT_NAME and JEST_JUNIT_OUTPUT_DIR configurations which give you the ability to specify a different filename and location respectively for this purpose.

@andrcuns
Copy link
Contributor Author

andrcuns commented Oct 22, 2019

@palmerj3 Indeed, I did not think about folks relying on the default value. Can we make it as a separate option? Something like JEST_JUNIT_UNIQUE_OUTPUT_NAME=true?

The problem with JEST_JUNIT_OUTPUT_NAME, is that if you have multiple commands in single CI job, each of those commands would have to be prefixed with this variable like JEST_JUNIT_OUTPUT_NAME=something.xml jest ..., JEST_JUNIT_OUTPUT_NAME=something2.xml jest ... which is a bit inconvenient I think.

But our problem is actually with the mentioned knapsack pro, it splits runs in to batches programmatically and there is no way to feed it unique values via ENV variable. So junit.xml ends up with values from the last batch only.

Another solution would be to merge result if the file already exists, but I don't think it's a good one.

Similar approach to what I am proposing, is used by Cucumber junit reporter for example.

@palmerj3
Copy link
Collaborator

@andrcuns I wouldn't be against adding it as an option as long as it's turned off by default.

But before we do that I would like to ask you to try something. I dug through the knapsack code a bit and it seems it might be possible to pass along some info from knapsack to jest-junit so you get a unique filename.

Knapsack sets an environment variable process.env.KNAPSACK_PRO_CI_NODE_INDEX which you might be able to use like this when calling jest-junit.

Sample jest.config.js file:

const uniqueId = process.env.KNAPSACK_PRO_CI_NODE_INDEX || '';
module.exports = {
  reporters = ['default', [ "jest-junit", { outputName: `junit${uniqueId}.xml` } ]]
};

I don't have a knapsack account to test this with but it appears something similar should work either using this via environment variables or with the jest config like I showed.

Let me know how that goes.

@andrcuns
Copy link
Contributor Author

@palmerj3 variable KNAPSACK_PRO_CI_NODE_INDEX is set once per CI node, so it will be exactly the same for all of the executions.

I could most likely generate some unique name just by using some uuid like in Your example, but I don't even add jest-junit reporter in jest.config.js because the only place it is used is CI, so I would like to keep using it by simply providing it as CLI arg --reporters=jest-junit.

@palmerj3
Copy link
Collaborator

Yes it's once per CI node but isn't that what you want? It is a unique identifier for each batch of tests that knapsack runs. The one that is shared between all runs is ciNodeBuildId.

Perhaps I'm not understanding your use-case if you need something beyond that or I don't understand knapsack well enough.

@andrcuns
Copy link
Contributor Author

Nop, the issue is not that there is a batch per CI node (which wouldn't be an issue really), but that it also splits test execution inside this single node in to sort of sub batches, it doesn't execute all of the tests as single jest run. I haven't looked in code why (most likely related to algorithms to evenly split tests by execution time).

For example:
I have a total test suite of 600 test files and I split execution in to 2 CI nodes. Each node would need to execute 300 test files (assuming they all take same time to execute). What happens is, knapsack pro first runs around 150 tests via jest programmatically and then runs remaining 150 test files. So what happens in the end, I get xml junit report only for the last 150 tests for both first and second CI nodes, because report for first 150 tests was overwritten.

@palmerj3
Copy link
Collaborator

I see. Thanks for explaining that to me.

Feel free to amend this PR and add the option for a unique filename that is off by default.

Thank you

@andrcuns
Copy link
Contributor Author

@palmerj3 Added separate option JEST_JUNIT_UNIQUE_OUTPUT_NAME which is false by default thus not affecting current default behaviour.

@palmerj3
Copy link
Collaborator

Looks good to me thanks for the PR!

@palmerj3 palmerj3 merged commit 414ddbb into jest-community:master Oct 22, 2019
@palmerj3
Copy link
Collaborator

@andrcuns ok this change has been published in v9.0.0 https://github.com/jest-community/jest-junit/releases/tag/v9.0.0

dbjorge pushed a commit to microsoft/axe-pipelines-samples that referenced this pull request Oct 25, 2019
Bumps [jest-junit](https://github.com/jest-community/jest-junit) from 8.0.0 to 9.0.0.
<details>
<summary>Release notes</summary>

*Sourced from [jest-junit's releases](https://github.com/jest-community/jest-junit/releases).*

> ## v9.0.0
> Ability to generate unique filenames for junit.xml by [@&#8203;andrcuns](https://github.com/andrcuns) [jest-community/jest-junit#108](https://github-redirect.dependabot.com/jest-community/jest-junit/pull/108)
</details>
<details>
<summary>Commits</summary>

- [`1ee5b29`](jest-community/jest-junit@1ee5b29) Merge pull request [#109](https://github-redirect.dependabot.com/jest-community/jest-junit/issues/109) from palmerj3/v9.0.0
- [`9cc91c2`](jest-community/jest-junit@9cc91c2) v9.0.0
- [`414ddbb`](jest-community/jest-junit@414ddbb) Merge pull request [#108](https://github-redirect.dependabot.com/jest-community/jest-junit/issues/108) from andrcuns/uniq_xml_name
- [`325abed`](jest-community/jest-junit@325abed) Add separate option to generate unique file names for xml report files
- [`32ba516`](jest-community/jest-junit@32ba516) Remove unsupported node versions, update readme
- [`2a60ded`](jest-community/jest-junit@2a60ded) Generate unique xml file by default
- See full diff in [compare view](jest-community/jest-junit@v8.0.0...v9.0.0)
</details>
<br />

[![Dependabot compatibility score](https://api.dependabot.com/badges/compatibility_score?dependency-name=jest-junit&package-manager=npm_and_yarn&previous-version=8.0.0&new-version=9.0.0)](https://dependabot.com/compatibility-score.html?dependency-name=jest-junit&package-manager=npm_and_yarn&previous-version=8.0.0&new-version=9.0.0)

Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting `@dependabot rebase`.

[//]: # (dependabot-automerge-start)
[//]: # (dependabot-automerge-end)

---

<details>
<summary>Dependabot commands and options</summary>
<br />

You can trigger Dependabot actions by commenting on this PR:
- `@dependabot rebase` will rebase this PR
- `@dependabot recreate` will recreate this PR, overwriting any edits that have been made to it
- `@dependabot merge` will merge this PR after your CI passes on it
- `@dependabot squash and merge` will squash and merge this PR after your CI passes on it
- `@dependabot cancel merge` will cancel a previously requested merge and block automerging
- `@dependabot reopen` will reopen this PR if it is closed
- `@dependabot close` will close this PR and stop Dependabot recreating it. You can achieve the same result by closing it manually
- `@dependabot ignore this major version` will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself)
- `@dependabot ignore this minor version` will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself)
- `@dependabot ignore this dependency` will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself)
- `@dependabot badge me` will comment on this PR with code to add a "Dependabot enabled" badge to your readme

Additionally, you can set the following in the `.dependabot/config.yml` file in this repo:
- Update frequency
- Automerge options (never/patch/minor, and dev/runtime dependencies)
- Out-of-range updates (receive only lockfile updates, if desired)
- Security updates (receive only security updates, if desired)



</details>
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