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

#1256 Fix duration being multiplied by 1000000 in JSON reporter #1257

Merged
merged 1 commit into from Nov 13, 2019

Conversation

dawn-minion
Copy link
Member

@dawn-minion dawn-minion commented Nov 13, 2019

The duration (which was already in nanoseconds) was being multiplied by 1000000 in src/formatter/json_formatter.js. This lead to the duration being reported in femtoseconds instead of nanoseconds, as it was in Cucumber 5.x. I'm not sure why this addition was added, but, this PR fixes the behaviour and returns the duration to the previously correct value.

EDIT: as noted below, this error was introduced in 6.0.0, which changed the internal reporting from milliseconds to nanoseconds. The JSON formatter was not updated when the change happened.

Fixes #1256

@dawn-minion dawn-minion changed the title #1256 Fix duration being multiplied by 100000 #1256 Fix duration being multiplied by 100000 in JSON reporter Nov 13, 2019
@dawn-minion dawn-minion changed the title #1256 Fix duration being multiplied by 100000 in JSON reporter #1256 Fix duration being multiplied by 1000000 in JSON reporter Nov 13, 2019
@dawn-minion
Copy link
Member Author

So looks like the unit tests disagree with me. I'm not sure why this multiplication was added in the first place - is this a remnant of converting milliseconds to nanoseconds? That matches the number here.

The duration was being multiplied by 1000000 in
src/formatter/json_formatter.js. This lead to the
duration being reported in femtoseconds instead of
nanoseconds, as it was in Cucumber 5.x.

Commit also updates the mocha tests for JSON reporter
@dawn-minion
Copy link
Member Author

From looking back at the code, that indeed seems to be the case. I've updated the unit tests here accordingly with a nanosecond version instead.

@charlierudolph
Copy link
Member

Good catch, yes you are correct in your findings. Forgot about that internal change when we restored the JSON formatter.

@charlierudolph charlierudolph merged commit 7e5773b into cucumber:master Nov 13, 2019
@aslakhellesoy
Copy link
Contributor

Hi @dawn-minion,

Thanks for your making your first contribution to Cucumber, and welcome to the Cucumber committers team! You can now push directly to this repo and all other repos under the cucumber organization! 🍾

In return for this generous offer we hope you will:

  • ✅ Continue to use branches and pull requests. When someone on the core team approves a pull request (yours or someone else's), you're welcome to merge it yourself.
  • 💚 Commit to setting a good example by following and upholding our code of conduct in your interactions with other collaborators and users.
  • 💬 Join the community Slack channel to meet the rest of the team and make yourself at home.
  • ℹ️ Don't feel obliged to help, just do what you can if you have the time and the energy.
  • 🙋 Ask if you need anything. We're looking for feedback about how to make the project more welcoming, so please tell us!

On behalf of the Cucumber core team,
Aslak Hellesøy
Creator of Cucumber

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.

Incorrect duration in 6.0.4 JSON report
3 participants