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

docs: add duplicate steps example and "Motivation" README #642

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

noodnik2
Copy link

@noodnik2 noodnik2 commented Sep 29, 2024

🤔 What's changed?

  • Added a sub-folder inside of _examples to describe & illustrate the "problem" we're facing;
  • A "stepchecker" tool to help detect when it's present, and;
  • A proposed "solution" to help us deal with it.

⚡️ What's your motivation?

The motivation and a more detailed description of the example case is described in the accompanying README file.

🏷️ What kind of change is this?

Just an example, which may be of use or inspiration to others facing a similar situation.

♻️ Anything particular you want feedback on?

Yes, everything written above and in the included README file.

📋 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.

Name: t.Name(),
ScenarioInitializer: func(context *godog.ScenarioContext) {
// NOTE: loading implementations of steps for different scenarios here
(&cloggedDrain{}).addCloggedDrainSteps(context)
Copy link
Author

Choose a reason for hiding this comment

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

The "clogged drain" scenario (above) works fine until the "flat tire" scenario (below) is added (e.g., in a subsequent commit).

The "I fixed it" step from the newly added flatTire scenario silently overrides the matching step in the previously defined step from the cloggedDrain scenario. The writer of the clogged drain scenario will have to investigate why his/her scenario suddenly stops working.

We'd like the tool to support a distributed development workflow where developers can work on scenarios independently, without having to take into account (and reuse) all pre-existing, matching steps.


func (ft *flatTire) addFlatTireSteps(ctx *godog.ScenarioContext) {
ctx.Step(`^I ran over a nail and got a flat tire$`, ft.gotFlatTire)
ctx.Step(`^I fixed it$`, ft.iFixedIt)
Copy link
Author

Choose a reason for hiding this comment

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

NOTE: the text of this new (or the old, pre-existing) step could be something less obviously matching - e.g., ^(.+)\s+fixed\s+(.+)$, making it harder for the second developer to spot and re-use the step instead of writing their own.

Copy link

codecov bot commented Oct 3, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 80.27%. Comparing base (153db4e) to head (54a569f).
Report is 16 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #642      +/-   ##
==========================================
- Coverage   83.21%   80.27%   -2.94%     
==========================================
  Files          28       40      +12     
  Lines        3413     3148     -265     
==========================================
- Hits         2840     2527     -313     
- Misses        458      502      +44     
- Partials      115      119       +4     

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

@Johnlon
Copy link
Member

Johnlon commented Oct 12, 2024

The text from the first para ... "Running the tests for the two Scenarios separately (e.g., using a separate godog.TestSuite)
could "solve" the problem, as matching the common Step text to its scenario-specific Function
would then be unambiguous within the Scenario-specific testing run context. However, a hard requirement
within our build pipeline requires a single "cucumber report" file to be produced as evidence
of the success or failure of all required test Scenarios. "

.... seems contradict the proposed solution

}
}

func collectGoSteps(goFilePath string, steps map[string]*StepMatch) error {
Copy link
Member

Choose a reason for hiding this comment

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

I dont know if this static parser will spot the cases where the ScenarioContext has been wrapped.
I've seen (and I use) a scheme where folk register steps via a proxy wrapper of the ScenarioContext, that adds generic behaviours to the steps being registered.

Copy link
Author

Choose a reason for hiding this comment

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

Nice catch! 👍

If you provide an example, I'll work up a test case to fix.

Do you have a fix strategy in mind?

@noodnik2
Copy link
Author

The text from the first para ... "Running the tests for the two Scenarios separately (e.g., using a separate godog.TestSuite)...

.... seems contradict the proposed solution

Hmm, okay I guess that didn't come out so clearly then.

What I'm trying to say is that by testing Scenarios individually (aka separately), we can work around the problem of ambiguity; but, that causes the problem (for us) of producing a single report covering all Scenarios, given that godog produces a separate report for each invocation of the encapsulating godog.TestSuite. Looking at the "problem" this way lead me to the proposed solution (see folder) of combining multiple reports into one.

Suggestions welcome for a clearer wording to express this!

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.

2 participants