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

Proposal: option for UsageFormatter to enable deterministic output #2439

Open
badeball opened this issue Nov 5, 2024 · 5 comments
Open

Proposal: option for UsageFormatter to enable deterministic output #2439

badeball opened this issue Nov 5, 2024 · 5 comments
Labels
✅ accepted The core team has agreed that it is a good idea to fix this ⚡ enhancement Request for new functionality

Comments

@badeball
Copy link
Member

badeball commented Nov 5, 2024

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

I have a slight problem testing the messages implementation of the non-associated Cypress implementation that I develop and maintain. Due to the chaotic and messy nature of Cypress plugin development, almost every test is an e2e test which invokes Cypress and verify some effect. For messages, this entails invoking the usage, json and html formatter and asserting their output, as seen eg. here.

The UsageFormatter sorts lines and its output based on execution time of steps.

For these test to be deterministic, I have to patch the usage formatter (or rather one of its helper methods), as seen here. I have no way of injecting a mock implementation of a clock, like done in the usage formatter's unit tests.

Unfortunately, patching dependencies using a typical postinstall script during local development and not have it invoked by consumers of the library, is less than trivial with npm, let alone all other package managers, as can be seen here.

✨ What's your proposed solution?

I'm wondering what you think about adding an option (possibly internal) to the UsageFormatter, to make its output deterministic (in regards to sorting lines). I would write the implementation and tests. What do you think?

⛏ Have you considered any alternatives or workarounds?

I could add actual and suffuciently large interrupts in each step, in order to be sure of the actual outcome, but I avoid that like the plague and I have yet to resort to such.

📚 Any additional context?

No response

@badeball badeball added the ⚡ enhancement Request for new functionality label Nov 5, 2024
@davidjgoss
Copy link
Contributor

davidjgoss commented Nov 19, 2024

I think deterministic ordering makes sense. Doing so by order we saw stepDefinition messages and then by order we saw pickle messages would seem to work?

I'm going to tag @mpkorstanje and @luke-hill since cucumber-jvm and cucumber-ruby also have a usage formatter and we want to be aligned on what we're doing as they will soon be rewritten as standalone packages that adhere to a common spec, as we have done with https://github.com/cucumber/junit-xml-formatter. (Side note, I'm working on a docs for dealing with that new style of formatter, and will tag you for review - keen to get your take on how good/bad it will be for the Cypress plugin.)

@mpkorstanje
Copy link
Contributor

mpkorstanje commented Nov 19, 2024

I do think we should retain duration as the default order. It makes sense to have the slowest steps on top of the output.

But having an option to apply a different order should also be possible. For example location or step text/pattern. This should satisfy the need for a determistic output.

I don't think first execution order has much practical use.

@luke-hill
Copy link

So long as we have a bit of hand holding as/when I go to do my first rewrite of a formatter (As it's an area of the codebase I've not worked on), I should be fine. I don't envisage it being a huge piece of work as usage is one of the smaller formatters.

@davidjgoss
Copy link
Contributor

Thanks all, so then @badeball I think we're fine to add ordering by location rather than duration, with an option like:

{
  "formatOptions": {
    "usage": {
      "order": "LOCATION"
    }
  }
}

@davidjgoss davidjgoss added the ✅ accepted The core team has agreed that it is a good idea to fix this label Nov 23, 2024
@badeball
Copy link
Member Author

Excelent, thanks! I'm pretty busy with my bachelor thesis atm and thus won't be starting with right away, but I'm looking forward to removing some of the hackish stuff in own my project (tm).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
✅ accepted The core team has agreed that it is a good idea to fix this ⚡ enhancement Request for new functionality
Projects
None yet
Development

No branches or pull requests

4 participants