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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature/junit reporter #1187

Merged
merged 8 commits into from
Dec 17, 2023
Merged

Conversation

awinder
Copy link
Contributor

@awinder awinder commented Dec 8, 2023

Description

This adds a new flag to bru run, the --format flag. It defaults to "json", which is the current output file format, while also implementing a "junit" option that will produce JUnit-formatted XML.

There are some opinionated implementation choices which I don't have strong opinions about:

  1. Using the bru filename (minus .bru) for test suite names in the junit markup
  2. Using a structure where the filename becomes a JUnit test suite, and then assertions + tests become test cases inside the suite
  3. JUnit tests (often? must?) supply a time attribute, and I had access to timing details on the .bru file / test suite level, but not the test case level. For implementation simplicity what I did was I used timing details from the .bru file to set the time attribute on the test suite, and then I divided it equally for the child testcases (which are the assertions / individual tests). This seemed accurate enough while hopefully not breaking some tooling that expects the time attr to be present

Also, I don't think I messed anything up applying a new npm module for xml writing (and chose a module which was already being required outside of the cli package), but the package-lock.json file updated widely so probably best to double-check there. I also removed a @jest/globals require in the test file because I couldn't get that to work locally, I can add that back, but I'm pretty sure it's unnecessary? And of course happy to address any feedback.

MOST IMPORTANTLY: THANK YOU so much for your work on Bruno, this project is awesome. I have now been burned twice on tools in this family (once with Swagger, and now again with Insomnia) adopting cloud policies that will never be acceptable as a business risk. I want to figure out how to champion sponsorship for this project but until then, consider this a small token of appreciation 馃槃

Contribution Checklist:

  • The pull request only addresses one issue or adds one feature.
  • The pull request does not introduce any breaking changes
  • I have added screenshots or gifs to help explain the change if applicable.
  • I have read the contribution guidelines.
  • Create an issue and link to the pull request.

Note: Keeping the PR small and focused helps make it easier to review and merge. If you have multiple changes you want to make, please consider submitting them as separate pull requests.

Publishing to New Package Managers

Please see here for more information.

result.assertionResults &&
result.assertionResults.forEach((assertion) => {
const testcase = {
'@name': `${assertion.lhsExpr} ${assertion.rhsExpr}`,
Copy link

Choose a reason for hiding this comment

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

How about adding the operator to the name, I think that is important to see the actual assertion that is tested here.

Copy link
Contributor Author

@awinder awinder Dec 11, 2023

Choose a reason for hiding this comment

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

In testing this it seems like the operator is already included in the rhsExpr field? When I ran this against some of my existing test suites I got results like res.body.array_field.length gt 0 or res.body[0].field_name isString. Are there some conditions where the operator isn't included in the rhsExpr?

@helloanoop
Copy link
Contributor

Thanks for the kind words @awinder 馃挍

PR looks good to me. I would recommend once change before we merge this
Let's move the junit reporter logic to a separate file.
I am thinking of a structure like

src
  |--commands
  |--runner
  |--reporters
    |--junit.js
  |--utils

There are some conflicts since I merged some PRs. Please sync your fork with the main.

@awinder
Copy link
Contributor Author

awinder commented Dec 15, 2023

Awesome, good idea. I refactored this & cleared up the merge conflicts but let me know if there's anything else!

@helloanoop helloanoop merged commit e3865d4 into usebruno:main Dec 17, 2023
2 checks passed
@helloanoop
Copy link
Contributor

Merged!

Thank you @awinder !

@helloanoop
Copy link
Contributor

@awinder I have released Bru CLI version 1.3.0

@AkkafaOguz
Copy link

AkkafaOguz commented Dec 25, 2023

Hi @awinder, I hope you're well. First of all thanks for your great effort!
I want to be sure that the command for format output feature is "bru run <folderName> --env <envName> --output response.xml --format junit". Because when I use this command, I got "Unknown argument: format" message.
Thanks for your interest and time.

@awinder
Copy link
Contributor Author

awinder commented Jan 2, 2024

Hi @awinder, I hope you're well. First of all thanks for your great effort! I want to be sure that the command for format output feature is "bru run <folderName> --env <envName> --output response.xml --format junit". Because when I use this command, I got "Unknown argument: format" message. Thanks for your interest and time.

Hi @AkkafaOguz, you might need to upgrade the bru CLI to the latest version. Check bru --version and make sure it's > 1.3.0. Then you can run npm update -g @usebruno/cli if you're out-of-date to pick up this feature. Your command works locally after I removed & reinstalled so I'm pretty sure this is a version issue.

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.

None yet

4 participants