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

GitHub Test Annotations #648

Merged
merged 40 commits into from
Nov 16, 2021
Merged

GitHub Test Annotations #648

merged 40 commits into from
Nov 16, 2021

Conversation

samchungy
Copy link
Contributor

@samchungy samchungy commented Nov 7, 2021

Resolves #638

Description

Adds a custom Jest Reporter which implements the new GitHub annotations API. This adds annotations to the skuba test command.

This will create a separate check run per jest.config displayName

@changeset-bot
Copy link

changeset-bot bot commented Nov 7, 2021

🦋 Changeset detected

Latest commit: ec0ce9e

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
skuba Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@samchungy
Copy link
Contributor Author

samchungy commented Nov 7, 2021

There's a slight issue with this approach and will need some good documentation around it.

At the moment if a project uses only a single jest.config file but uses different CLI commands for different tests. eg. yarn test --testPatterns *.int.test.ts and yarn test --testPatterns *.unit.test.ts. There will only be one check run with the name skuba test created with the latter skuba test call overwriting the results of the first. I cannot think of a simple workaround for this unless we change our approach and call to get all the check runs beforehand and create one with a different name eg. skuba test 2. What would happen if they ran two tests at the same time?

I would like to encourage users to create separate jest.config files for different test types eg. for the above scenario we could create 2 separate jest config files.

Eg.
jest.config.int.ts

export default Jest.mergePreset({
    displayName: 'integration',
    testPatterns: ['*.int.test.ts']
})

jest.config.ts

export default Jest.mergePreset({
    displayName: 'unit',
    testPatterns: ['*.unit.test.ts']
})

This will create two separate check runs skuba/test (integration) and skuba/test (unit).

Alternative, you could choose to not label one with a displayName.

Eg.
jest.config.int.ts

export default Jest.mergePreset({
    displayName: 'integration',
    testPatterns: ['*.int.test.ts']
})

jest.config.ts

export default Jest.mergePreset({
    testPatterns: ['*.unit.test.ts']
})

This will alsos create two separate check runs skuba/test (integration) and skuba/test.

Copy link
Member

@72636c 72636c left a comment

Choose a reason for hiding this comment

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

Exciting 🙌

src/cli/test/reporters/github/index.ts Outdated Show resolved Hide resolved
jest-preset.js Outdated Show resolved Hide resolved
src/cli/test/reporters/github/index.ts Outdated Show resolved Hide resolved
src/cli/test/reporters/github/annotations.ts Outdated Show resolved Hide resolved
src/cli/test/reporters/github/annotations.ts Outdated Show resolved Hide resolved
@72636c
Copy link
Member

72636c commented Nov 7, 2021

I think this displayName-based approach makes sense. We can either encourage users to have multiple config files with distinct displayNames, or a single config file with multiple projects and associated displayNames (if the latter, we should think about how we can better support that style in Jest.mergePreset).

We could hash process.cwd() and process.argv if we wanted to completely avoid clobbering check runs, but I wouldn't bother with that unless we see this displayName approach fall over in practice. Keep in mind this won't enable itself on existing repos until peeps update them to propagate the necessary prerequisites, and they can fix up configs while they are there anyway.

annotations,
conclusion,
summary,
title: `${build} ${isOk ? 'passed' : 'failed'}`,
Copy link
Member

Choose a reason for hiding this comment

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

Not for this PR, but I'd be interested to see whether we can extend this with coverage in future. It would be neat if we could compare against the default branch a la Codecov; could we try to retrieve details of the equivalent check run on the head commit for comparison?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@72636c 72636c mentioned this pull request Nov 7, 2021
@samchungy samchungy marked this pull request as ready for review November 15, 2021 23:49
@samchungy samchungy requested a review from a team as a code owner November 15, 2021 23:49
docs/cli/test.md Outdated Show resolved Hide resolved
Copy link
Member

@72636c 72636c left a comment

Choose a reason for hiding this comment

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

🙇

Comment on lines +27 to +28
// Create a check run per display name.
// Run in series to reduce the likelihood of exceeding GitHub rate limits.
Copy link
Member

Choose a reason for hiding this comment

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

Just double checking that this was the intent of running in series?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correcto, you wake up early as 👀

Co-authored-by: Sam Chung <[email protected]>
@72636c 72636c merged commit 6311f20 into master Nov 16, 2021
@72636c 72636c deleted the github-test-annotations branch November 16, 2021 16:56
@github-actions github-actions bot mentioned this pull request Nov 16, 2021
@samchungy samchungy restored the github-test-annotations branch November 16, 2021 19:51
@72636c 72636c deleted the github-test-annotations branch November 25, 2021 00:52
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.

GitHub Annotations for test
2 participants