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

feat(reporter): implement Jest reporter #80

Merged
merged 21 commits into from
Mar 29, 2021

Conversation

lkipke
Copy link
Contributor

@lkipke lkipke commented Mar 17, 2021

Summary

This leverages the jest packages and tap-parser to create a Jest-style reporter.

Screenshots

Default reporter

Screen Shot 2021-03-17 at 1 18 18 PM

Verbose reporter

Screen Shot 2021-03-17 at 1 18 41 PM

Failing due to assert

Screen Shot 2021-03-17 at 1 17 55 PM

Failing due to runtime exception

In test case

Screen Shot 2021-03-17 at 1 19 09 PM

In source code

Screen Shot 2021-03-17 at 1 19 47 PM

Print statements

Code:

m.it("passing test case", sub()
    print "here's a print statement with an object: " { some: "field" }
    m.assert.equal("abcd", "abcd", "they are equal")
end sub)

m.it("passing test case", sub()
    print "hello, i am but a humble print statement"
    m.assert.deepEquals({ foo: "abcd" }, { foo: "abcd" }, "they are equal")
end sub)

Screen Shot 2021-03-17 at 1 09 16 PM

@lkipke lkipke added the enhancement New feature or request label Mar 17, 2021
@lkipke lkipke requested review from sjbarag and alimnios72 March 17, 2021 20:01
@lkipke lkipke self-assigned this Mar 17, 2021
// Flatten console output because tap-parser splits output by newline in the "extra" event.
// We don't know which lines are supposed to go together, so just print them all together.
if (this.currentResults.console) {
this.currentResults.console = [
Copy link
Contributor Author

@lkipke lkipke Mar 17, 2021

Choose a reason for hiding this comment

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

This was the best solution I could think of. It's a little wonky, but it's not too far off from how Jest prints console.log statements in JS tests. The main problem is we don't have any stack trace for the logs.

Jest reporter for roca (what this PR implements):
Screen Shot 2021-03-17 at 1 24 11 PM

Actual jest reporter for JS (I put some console.logs in the roca project's JS test suite):
Screen Shot 2021-03-17 at 1 11 47 PM

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is fine as long as we get stack traces for errors which seems like we do already 😄

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed! Since we treat brs (specifically its print statements) as an opaque box from the roca perspective, I'm fine with losing that for now. It might be possible to add an eventEmitter that handles writes to stdout instead of calling interpreter.stdout.write() directly from within brs's implementation of print, but that's not necessary here at all. It'd be neat to consider for the future though — especially in combination with a --quiet in brs to suppress writes to stdout?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It'd be neat to consider for the future though — especially in combination with a --quiet in brs to suppress writes to stdout?

I agree, that'd be great!

// Flatten console output because tap-parser splits output by newline in the "extra" event.
// We don't know which lines are supposed to go together, so just print them all together.
if (this.currentResults.console) {
this.currentResults.console = [
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is fine as long as we get stack traces for errors which seems like we do already 😄

Copy link
Contributor

@alimnios72 alimnios72 left a comment

Choose a reason for hiding this comment

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

Thanks for the changes

Copy link
Contributor

@sjbarag sjbarag left a comment

Choose a reason for hiding this comment

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

For safety's sake it might be worth keeping --reporter spec as the default for a release or two while we opt-into --reporter jest to shake out any hidden bugs, but if you're comfortable with it I'm down to give this a try!

Excellent work @lkipke — I'm excited to get this out to the world!

.map((entry) => entry.message)
.join(""),
origin: "", // TODO: figure out how to get the stack trace
type: "print" as any, // a little hack because brightscript uses `print` instead of `console.log`
Copy link
Contributor

Choose a reason for hiding this comment

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

Aw bummer it reports as console.print instead of just print? So close!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I know, so close 😢 And we have no control over the outputted string here, either, so we can't use replace or anything, sadly.

@lkipke lkipke merged commit 49161c7 into hulu:main Mar 29, 2021
@lkipke lkipke deleted the implement-jest-reporter branch March 29, 2021 21:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants