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

fix logFailedTestCases for RerunFormatter #2292

Merged
merged 7 commits into from
Jun 22, 2023

Conversation

furiel
Copy link
Contributor

@furiel furiel commented Jun 17, 2023

Referring to #2290

What's changed?

  • RerunFormatter used to log failed attempts even if the attempt was retried. For example, it was possible that a testcase might have been fixed with one of the retry attempts, but still was logged due to earlier failing attempt. From now, RerunFormatter does not log attempts that are retried.
  • Some minor quality of life changes for users who would like to extends RerunFormatter.

What's your motivation?

I am using retry to stabilize tests of a distributed system. It was possible (for example right after a deployment), that the first couple of test attempts are failed against a service. I am also using RerunFormatter to log the failures. However, as RerunFormatter logged the failed attempts too, it was difficult to find out what testcases I needed to look at in case of a regression.

I attempted to extend RerunFormatter according to my needs, which turned out to be more complex than I anticipated.

I created an issue and this patchet about both behavior of the RerunFormatter, and the challenges of it's customization.

What kind of change is this?

  • 🐛 RerunFormatter's logFailedTestCases output is changed.
  • 🏦 Minor refactors and changes to RerunFormatter to simplify extends-ing from it.

Anything particular you want feedback on?

This is my first time I am working with typescript/javascript. I appreciate any feedbacks, let it be small or complex. However, I may need a bit more guidance due to my infamiliarity with the language. Do not hesitate to overexplain the change requests. :)
I also set Allow edits by maintainers if you feel something is easier for you to change, instead of guiding me through.

Checklist:

  • I agree to respect and uphold the Cucumber Community Code of Conduct
  • I've changed the behaviour of the code
    • I have added/updated tests to cover my changes.
  • Users should know about my change
    • I have added an entry to the "Unreleased" section of the CHANGELOG, linking to this pull request.

@coveralls
Copy link

coveralls commented Jun 17, 2023

Coverage Status

coverage: 98.499% (+0.002%) from 98.497% when pulling d6db66d on furiel:unstable-tests into 8fece83 on cucumber:main.

features/retry.feature Outdated Show resolved Hide resolved
Copy link
Contributor

@michael-lloyd-morris michael-lloyd-morris left a comment

Choose a reason for hiding this comment

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

Overall looks great. Some minor doc fixing in the changelog, but that could be fixed during merge I think.

CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
Copy link
Contributor

@davidjgoss davidjgoss left a comment

Choose a reason for hiding this comment

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

Thanks for doing this, looks good! Just a few things I've noted that would be good to change. Happy to help if you need it.

src/index.ts Outdated Show resolved Hide resolved
CHANGELOG.md Show resolved Hide resolved
features/retry.feature Show resolved Hide resolved
Before doing my modifications, I wanted to add a test about the
incorrect behavior discussed in
cucumber#2290

I will update this test in a later patch that fixes the issue.

Signed-off-by: Antal Nemes <antal.nemes.hu@gmail.com>
This change makes followup patch a bit more readable.

Signed-off-by: Antal Nemes <antal.nemes.hu@gmail.com>
Prior this patch, RerunFormatter should log unstable tests: tests that
failed at any retry attempt. Checking the `willBeRetried` value makes
sure we will not log failures for rerun attempts that will be retried.

Referring: cucumber#2290 for
more information.

Signed-off-by: Antal Nemes <antal.nemes.hu@gmail.com>
Introducing `getFailureMap()` and `formatFailedTestCases()`
to decouple console logging with the calculation. This would help
users who want to customize RerunFormatter

For more information: cucumber#2290

Signed-off-by: Antal Nemes <antal.nemes.hu@gmail.com>
separator is not part of a complex logic, neither a complex
structure. Users may want to reuse it for customizing RerunFormatter.

For more details: cucumber#2290

Signed-off-by: Antal Nemes <antal.nemes.hu@gmail.com>
Signed-off-by: Antal Nemes <antal.nemes.hu@gmail.com>
@davidjgoss davidjgoss merged commit 4c2443a into cucumber:main Jun 22, 2023
@aslakhellesoy
Copy link
Contributor

Hi @furiel,

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.

6 participants