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(report): Include tap mocha reporter by default #9

Merged
merged 2 commits into from Oct 11, 2019

Conversation

sjbarag
Copy link
Contributor

@sjbarag sjbarag commented Oct 8, 2019

Rather than hand-writing our own reporters, we can pipe brs's stdout directly through tap-mocha-reporter and leverage all of the reporters that it supports. That's more than enough to get us started, and allows roca users to write their own TAP reporters as desired!

@sjbarag sjbarag added the enhancement New feature or request label Oct 8, 2019
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.

Looks great! I'm wondering if we the test directory should be an optional argument, is there any reason why you defined at the brightscript level?

src/index.js Show resolved Hide resolved
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.

Some dependencies are using the wrong registry

package-lock.json Outdated Show resolved Hide resolved
@alimnios72
Copy link
Contributor

We'll need a version bump since there are breaking changes, right?

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.

I tried running a simple test but ran into some issues, comments below

resources/roca_main.brs Outdated Show resolved Hide resolved
resources/roca_main.brs Outdated Show resolved Hide resolved
This removes the now-unnecessary `file` subcommand and sets up a default
command with no subcommands.
@sjbarag
Copy link
Contributor Author

sjbarag commented Oct 11, 2019

We'll need a version bump since there are breaking changes, right?

Since we haven't hit 1.0 I think there's no expectation of the API being stable (at least according to the semver spec). We're coming up to 1.0 though! I think once we add fdescribe and xdescribe 😄

@sjbarag
Copy link
Contributor Author

sjbarag commented Oct 11, 2019

(rebased on top of #8 now that that's landed!)

@sjbarag
Copy link
Contributor Author

sjbarag commented Oct 11, 2019

Looks great! I'm wondering if we the test directory should be an optional argument, is there any reason why you defined at the brightscript level?

Only because I don't think we've configured the top-level brs.execute doesn't accept arguments just yet! Seems like something we'll inevitably want configurable though, so I'll file a quick issue for it!

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.

Looks great, 🚢 it

@sjbarag sjbarag merged commit 93312b3 into hulu:master Oct 11, 2019
@sjbarag sjbarag deleted the include-tap-mocha-reporter branch October 11, 2019 23:47
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.

None yet

2 participants