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(formatter): On concurrent execution, execute formatter at end of Scenario #645

Merged
merged 13 commits into from
Nov 8, 2024

Conversation

tigh-latte
Copy link
Member

@tigh-latte tigh-latte commented Oct 5, 2024

🤔 What's changed?

Add a new formatter type which only executes when Flush is called. Use this formatter when running concurrently.

Then while running concurrently, after the suite is copied for a scenario, we overwrite the copy's Formatter to use this new OnFlushWrap formatter.

At the end of runPickle, if we can Flush, we do.

⚡️ What's your motivation?

Fixes #643 , Scenario output is only printed at end of runPickle func when running concurrently.

🏷️ What kind of change is this?

  • 🐛 Bug fix (non-breaking change which fixes a defect)

♻️ Anything particular you want feedback on?

Any feedback on the general approach to solving this would be good.

📋 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.
  • My change requires a change to the documentation.
    • I have updated the documentation accordingly.
  • Users should know about my change
    • I have added an entry to the "Unreleased" section of the CHANGELOG, linking to this pull request.

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

@tigh-latte tigh-latte marked this pull request as draft October 5, 2024 13:45
@tigh-latte
Copy link
Member Author

one test is broken, Test_FormatterConcurrencyRun but on when running scenario_outline.feature, keep this as a draft until I work out why.

Copy link

codecov bot commented Oct 5, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 79.79%. Comparing base (153db4e) to head (b3b7d25).
Report is 23 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #645      +/-   ##
==========================================
- Coverage   83.21%   79.79%   -3.42%     
==========================================
  Files          28       41      +13     
  Lines        3413     4044     +631     
==========================================
+ Hits         2840     3227     +387     
- Misses        458      697     +239     
- Partials      115      120       +5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@@ -233,6 +233,21 @@ func (s *Storage) MustGetPickleStepResultsByPickleID(pickleID string) (psrs []mo
return psrs
}

// MustGetPickleStepResultsByPickleIDUntilStep will retrieve pickle step results by pickle id
// from 0..stepID for that pickle.
func (s *Storage) MustGetPickleStepResultsByPickleIDUntilStep(pickleID string, untilStepID string) (psrs []models.PickleStepResult) {
Copy link
Member Author

Choose a reason for hiding this comment

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

This only needed to be done for the Pretty formatter, so I wasn't sure if I should just do it after MustGetPickleStepResultsByPickleID in fmt_pretty.go, or create a function for it.

I've opted to create a function with a pretty bad name, so let me know if yous are happy with this.

@vearutop vearutop merged commit c5a88f6 into cucumber:main Nov 8, 2024
6 of 7 checks passed
@vearutop
Copy link
Member

vearutop commented Nov 8, 2024

Thank you!

@Johnlon
Copy link
Member

Johnlon commented Nov 8, 2024

if this is a new formatter please, can we please have fmt-output-test cases in line with the other formatters please .

consistent test patterns help other commiters helps avoid entropy

@tigh-latte
Copy link
Member Author

@Johnlon it isn't really a new formatter and instead is a wrapper of current formatters. All it does is delay the execution of the wrapped formatter until the end of the scenario, and this delayed execution is tested.

In an additional PR, I could put time into testing that the output of a wrapped formatter is as we would expect, but given the wrapper only provides behavioural changes, testing the behaviour as I've done here and using the output tests already in place is enough.

@tigh-latte tigh-latte deleted the fix/concurrent-logging branch November 10, 2024 17:34
@thomshutt
Copy link

@tigh-latte Thanks so much for adding this, I've been able to re-enable concurrency in my tests because of it!

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.

Running with concurrency shouldn't intermingle the test outputs
4 participants