Skip to content
This repository has been archived by the owner on May 8, 2023. It is now read-only.

Add custom reporter to produce Problem Matcher output #2

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

stefanbuck
Copy link
Owner

@stefanbuck stefanbuck commented Apr 24, 2020

Demo PR for jestjs/jest#10309

json-reporter.js Outdated
const col = item.location.column;
const message = item.failureMessages[0].replace(/\n/g, newLine);

console.log(`::error file=${name},line=${line},col=${col}::${message}`);
Copy link
Owner Author

Choose a reason for hiding this comment

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

The GitHub runner has some build in matchers and therefore all we have to do is producing output in this format actions/toolkit#260 (comment)

json-reporter.js Outdated Show resolved Hide resolved
json-reporter.js Outdated
if (item.status !== "failed") {
return;
}
const newLine = "%0A";
Copy link
Owner Author

Choose a reason for hiding this comment

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

We have to url encode new lines in order to produce a matcher message that can spawn over multiple lines

json-reporter.js Outdated

const line = item.location.line + 1;
const col = item.location.column;
const message = item.failureMessages[0].replace(/\n/g, newLine);
Copy link
Owner Author

Choose a reason for hiding this comment

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

We might need to iterate over failureMessages, but for this MVP its should be enough

json-reporter.js Outdated
);
}

const line = item.location.line + 1;
Copy link

Choose a reason for hiding this comment

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

+ 1? testLocation is for the it, not the expect. You're probably better of looking at the stack trace in the error

Copy link
Owner Author

Choose a reason for hiding this comment

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

I misread the documentation

Note that column is 0-indexed while line is not.
https://jestjs.io/docs/en/cli.html#--testlocationinresults

Anyway, thanks for the hint about the it this wasn't clear from the documentation. Will parse the error message as you suggested.

Copy link
Owner Author

Choose a reason for hiding this comment

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

I updated the implementation according to your comment. Should I try to get a Jest PR up? If you could point me to a location / package where I should make changes, that would be amazing. From a quick look it feels like it should be part of https://github.com/facebook/jest/tree/master/packages/jest-reporters/

Copy link

Choose a reason for hiding this comment

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

Part of reporters, yeah.

Auto-adding it would go somewhere here: https://github.com/facebook/jest/blob/faa52673f4d02ee1d273103ada9382a0805b54a4/packages/jest-core/src/TestScheduler.ts#L267-L298

We should probably wait a bit with that until we're happy, though

Copy link
Owner Author

@stefanbuck stefanbuck Apr 29, 2020

Choose a reason for hiding this comment

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

I'm already super happy with the result 😉

I want to get this feature in so I can deprecate a Danger.js plugin I wrote to present failing tests on a Pull Request. Also I think the Jest community will love this integration which works out of the box with every GitHub Action setup.


The image below shows a comment on a Pull Request generated by a custom Danger.js](https://github.com/danger/danger-js) plugin

@stefanbuck stefanbuck force-pushed the add-custom-reporter branch from e14b89a to daf2e9e Compare April 25, 2020 20:35
@SimenB
Copy link

SimenB commented Apr 27, 2020

Would be interesting to get some colors in the posted message. Is that supported? We could probably find some way to translate ANSI codes to html/css

@stefanbuck stefanbuck force-pushed the add-custom-reporter branch 2 times, most recently from 2d66ef7 to daf2e9e Compare April 28, 2020 22:00
@stefanbuck
Copy link
Owner Author

I tried a few different approaches to color the output, but none of them worked. I raised a feature request in the toolkit repo actions/toolkit#435.

@SimenB
Copy link

SimenB commented Apr 29, 2020

I guess we could generate some markdown and post that? That should take care of the diff. Would be nice to have some ansi->html translation though as coloring after the fact is painful

@stefanbuck
Copy link
Owner Author

I guess we could generate some markdown and post that?

As a comment on the Pull Request? This would require a dedicated Jest Action. The current solution works out of the box if jest runs on GitHub Action, no setup, no config.

@SimenB
Copy link

SimenB commented Apr 29, 2020

Ah, we can't dump markdown via a problem matcher?

@stefanbuck
Copy link
Owner Author

I would be surprised if it does. The Problem Matcher is meant to forward "terminal" output to GitHub's interface.

@stefanbuck stefanbuck force-pushed the add-custom-reporter branch from aa84fac to ca062e8 Compare April 29, 2020 23:15
@SimenB
Copy link

SimenB commented Apr 29, 2020

Ah, ok. Too bad, although I guess that could be abused somewhat to post images and what not

@SimenB
Copy link

SimenB commented Apr 29, 2020

My only remaining concern adding it as a default then is that there would be no easy way to turn it off if people don't want it. Except defining all reporters yourself. Maybe not a huge issue, though

@stefanbuck
Copy link
Owner Author

stefanbuck commented Apr 29, 2020

Markdown is not supported

image

@stefanbuck
Copy link
Owner Author

stefanbuck commented Apr 29, 2020

My only remaining concern adding it as a default then is that there would be no easy way to turn it off if people don't want it

What if we add a flag to Jest config like

{
  githubActionEnabled: true // (default: true)
}

or we could listen for an env var JEST_GITHUB_ACTION_REPORTER

@stefanbuck stefanbuck force-pushed the add-custom-reporter branch from ca062e8 to daf2e9e Compare April 29, 2020 23:21
@SimenB
Copy link

SimenB commented Apr 29, 2020

can it be made to dismiss earlier entries? I'm guessing no...

@stefanbuck
Copy link
Owner Author

Usually it does. Before a new run starts it removes all annotations. Looks like a bug caused by me force pushing

@stefanbuck
Copy link
Owner Author

I'll raise a new PR to ensure its working as expected

@stefanbuck
Copy link
Owner Author

stefanbuck commented Apr 29, 2020

On this PR three actions are running where it should be only one

image

@stefanbuck stefanbuck force-pushed the add-custom-reporter branch from daf2e9e to 82d50fd Compare April 29, 2020 23:33
@stefanbuck
Copy link
Owner Author

Fixed by recreating my last commit. I've noticed quite a few GitHub Actions problems when force pushing commits.

@jsg2021
Copy link

jsg2021 commented Jul 6, 2020

This is super exciting. I hope the momentum hasn't trailed off. :)

@stefanbuck
Copy link
Owner Author

I had a closer look at the Jest repository, but I have to admit that I was quite overwhelmed by the architecture given that I'm not a TypeScript developer. @SimenB what's the best way to get this thing moving? Should I raise an issue in the Jest repo and outline the main motivation for this feature and summarise everything I explored so far?

}

const [, line, col] = captureGroup;
console.log(`::error file=${testFilePath},line=${line},col=${col}::${message}`);
Copy link
Owner Author

Choose a reason for hiding this comment

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

This is the magic which turns a console.log into a GitHub annotation.

@xt0rted
Copy link

xt0rted commented Aug 23, 2020

I just started using a slightly modified version of this reporter for one of my projects. I'm using the issueCommand function from @actions/core which also handles the encoding, and I stripped the call stack since it's extra noise in this context.

The tests are running twice in my workflow, once in a build job and again in a code coverage job, so I set an env var on one of them and conditionally add the reporter so I don't get duplicate annotations.

I can post my changes if anyone's interested.

@mariusGundersen
Copy link

This is a fantastic simple solution to the problem. I see that it has been suggested in the jest repo but maybe it could be suggested in jest-community as well? And if they won't add it then maybe make an unofficial npm package. I might consider doing that myself, with probably more documentation than code as this seems to be a very common problem with lots of bad solutions based on some google searching.

@SimenB
Copy link

SimenB commented Oct 23, 2020

I'm happy to move the code for this over into the Jest repo (as long as it gets some tests), then we can figure out whether or not it should be enabled by default later

@stefanbuck
Copy link
Owner Author

@SimenB as mentioned earlier, I had a closer look at the Jest repository, but I have to admit that I was quite overwhelmed by the architecture. The actual code is quite simple, but I wonder how to integrate this into Jest best. @SimenB do you mind providing an outline which files are involved? That would be really helpful.

@SimenB
Copy link

SimenB commented Oct 27, 2020

Put it into https://github.com/facebook/jest/tree/master/packages/jest-reporters/src and export it from index.ts.

It needs to be ported to TypeScript and have some tests written for it - both unit and integration. As a start just open a PR and we can work together to land it 🙂

After it has landed we can figure how to document it, expose it to the user and whether or not it should be enabled by default when we think we are in a GH Actions run

@mariusGundersen
Copy link

I created an initial PR here: jestjs/jest#10741

@stefanbuck
Copy link
Owner Author

stefanbuck commented Oct 30, 2020

Hey @mariusGundersen, what is the purpose of this placeholder PR? Maybe I'm just confused because you called it placeholder. Anyway, I started working on this as well. I'm more than happy to collaborate on this. I used the SummaryReporter as a base which doesn't look much different from your solution stefanbuck/jest@9d5f65a

How should we proceed? Are you familiar with the integration part?

@SimenB
Copy link

SimenB commented Apr 26, 2022

shipped in jest 28, fwiw: https://jestjs.io/blog/2022/04/25/jest-28#github-actions-reporter

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants