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

An option to disable the performance counter on demand. #2086

Closed
verrantom opened this issue Jul 4, 2022 · 6 comments · Fixed by #2094
Closed

An option to disable the performance counter on demand. #2086

verrantom opened this issue Jul 4, 2022 · 6 comments · Fixed by #2094
Assignees
Labels
🐛 bug Defect / Bug ✅ accepted The core team has agreed that it is a good idea to fix this

Comments

@verrantom
Copy link

🤔 What's the problem you're trying to solve?

#1793 introduced a change whereby durations on the reports output as part of the cucumber_report.json can be decimal values. This is resulting in AWS CodeBuild's built in test report processor (written in Go) causing issues as the duration is no longer an integer.

✨ What's your proposed solution?

After speaking to @aurelien-reeves they suggested an option that would disable the performance counter on demand.

⛏ Have you considered any alternatives or workarounds?

An alternate workaround would be adding a specific step to our AWS CodeBuild pipeline to undo the changes made in the pull request linked above.

📚 Any additional context?

I raised this issue in the "help-cucumber-js" slack channel and spoke to Aurélien who suggested I raise this feature request.

This text was originally generated from a template, then edited by hand. You can modify the template here.

@davidjgoss
Copy link
Contributor

@verrantom thanks for raising!

From reading up a bit on CodeBuild's test reporting, looks like it parses and uses the (legacy) JSON formatter output. On that basis I think I'd be fine with just changing the JSON formatter to round to integer milliseconds - arguably we've made a breaking change there (by changing it to a decimal) and CodeBuild is just a prominent victim.

@aurelien-reeves what do you think?

@davidjgoss davidjgoss added the 🐛 bug Defect / Bug label Jul 4, 2022
@aurelien-reeves
Copy link
Contributor

Thanks for the request @verrantom 👍

@davidjgoss I am pretty sure that a lot of our users still rely a lot on the json formatter, and that they may have already taken advantage of the new accurate measures. That would be a regression to them.

And even if that is not the case, that would seem odd to not have the same value for duration from one formatter to another one.

I would prefer an option to deactivate the high precision measures.

@davidjgoss
Copy link
Contributor

davidjgoss commented Jul 5, 2022

@aurelien-reeves so like:

"formatOptions": {
  "json": {
    "highPrecisionTimestamps": false
  }
}

I'd be cool with that if we're making false the default - so anyone who wants to use high precision with the JSON formatter can enable it, but it's off by default so it's not broken by default on CodeBuild.

@aurelien-reeves
Copy link
Contributor

aurelien-reeves commented Jul 5, 2022

I was more thinking about a global option that would impact src/time.ts directly, and thus all formatters at once. Not only the JSON one.

And I would have suggest to opt-out of high-precision measures per default. But I would not object into opting-in for it. I think it was a good and reasonable improvement despite that regression with some third-party tools. And it was released as part of a new major version so we did not missed the breaking change in a minor or patched version here.

@davidjgoss davidjgoss added the ✅ accepted The core team has agreed that it is a good idea to fix this label Jul 19, 2022
@davidjgoss davidjgoss self-assigned this Jul 28, 2022
@davidjgoss
Copy link
Contributor

After digging into this a little today:

I think the best short-term course would be to Math.round the durations for the JSON formatter to fix that regression, and then do a more in-depth review of our timing code.

@davidjgoss
Copy link
Contributor

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🐛 bug Defect / Bug ✅ accepted The core team has agreed that it is a good idea to fix this
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants