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

Make Suite.runSteps Method public #167

Closed
ziprandom opened this issue Mar 26, 2019 · 5 comments
Closed

Make Suite.runSteps Method public #167

ziprandom opened this issue Mar 26, 2019 · 5 comments

Comments

@ziprandom
Copy link

ziprandom commented Mar 26, 2019

I'm using godog in a complex system testing scenario and would really like to more flexibly reuse parts of my feature files. I have been using the multistep feature but think it could be replaced by this functionality as it reuses the Suite to run gherkin scenarios which can be parsed from external files or gherkin.DocString without the restrictions on Datatables or DocStrings as arguments metioned here #89

func FeatureContext(s *godog.Suite) {
	s.Step(`^the "([^"]*)" passes$`, makeTheFeaturePasses(s))
}

func makeTheFeaturePasses(context *godeliatypes.Context) func(string) error {
	return func(featureFile string) error {
		file, err := os.Open(featureFile);
		if err != nil {
			return fmt.Errorf("couldn't open feature file: %v", err)
		}

                feature, err := gherkin.ParseFeature(file)
		if err != nil {
			return fmt.Errorf("couldn't parse feature file: %v", err)
		}

		for i := range feature.ScenarioDefinitions {
                        if d, ok := feature.ScenarioDefinitions[i].(*gherkin.Scenario); ok { 
				return context.Suite.RunSteps(d.Steps)
			} else { 
                                return fmt.Errorf("couldn't handle %v", feature.ScenarioDefinitions[i])
                        }
		}

		return nil
	}
}
Feature: Another Multi Step Feature

    Scenario: Run the external Feature
        Given the "reusable_backgrounds/preparation.feature" passes
@l3pp4rd
Copy link
Member

l3pp4rd commented Mar 26, 2019

Hi, could you explain what is the context you are using godog with? Godog uses godog to test itself, since I see you build steps to run feature files, maybe godog.SuiteContext is the more appropriate tool for such case, and instead you could contribute to that context which you can reuse. It has all kinds of steps in order to run feature files and make related assertions.

I want to be aware of reasoning for such changes, since that relates to public interface of godog which will need to be maintained

@ziprandom
Copy link
Author

ziprandom commented Mar 26, 2019

Hey @l3pp4rd thanks for your reply!

In my situation I want to address certain features scenarios from another feature that tests an app update. so basically I want to be able to comfortably select certain scenarios (as portions from the normal tests) to make sure they pass in the context of a recently updated service. So I would parse the respective feature file, run the steps of its background and then insert my update step before I proceed to the selected scenario.

I know this is a very special use case but I`m sure others exist in more complex scenarios. In our case this change helps keeping the feature files relatively short and expressive with good reuse of gherkin scenarios.

Thanks for the hint to godog.SuiteContext. Here we could implement a step or a method (which would still change the public api) that does the same but given our use case it's really not about testing our tests so the SuiteContext doesn't feel like the right place semantically.

With the RunSteps approach one could maybe even replace the nested step feature completely. An option to mute the step feedback could be all that's needed to reproduce the functionality with RunSteps.

Anyways I appreciate any help in getting the desired behaviour and understand your concerns regarding the api change.

thanks for this great tool!

@l3pp4rd
Copy link
Member

l3pp4rd commented Mar 28, 2019

I'll need to postpone such change, it requires more thinking. at the moment, run step fires all events to formatters and that may cause unexpected behavior, like for example, since these steps may be created dynamically, they are not related to any feature file and that is not an expected behavior godog may deal with including everyone using it. So far I do not think that I can accept such a change, concerning nested steps, they will be removed since that has too many flaws. I would suggest just to have some kind of a workaround for now. For example just combining that code logic from steps you are executing into a normal function and hitting it whenever reasonable from the step.

@ziprandom
Copy link
Author

I understand your reasoning and will find another solution for my use case.

However I hope you don't (although I understood here you do) plan to remove the Steps return value option for step functions before an alternative (like the one proposed here) is available. From the point of view of test case manageability it makes a huge difference whether I can chain my steps with gherkin or have to code the function calls from go.

For example one very important step in my ensemble is:

Background: 
  And the following steps run once for this feature: 
    """
    # expensive setup
    I reset the database
    I do other expensive
    """

@mxygem
Copy link
Member

mxygem commented Apr 30, 2021

Hey @ziprandom,

Thanks so much for taking the time to propose this idea and to submit a PR around it. As l3pp4rd mentioned, nested steps are considered an anti-pattern and will be removed. While your particular use-case isn't -exactly- that, it feels very close.

Since Gherkin is meant to be a declarative representation of a feature's behavior, things like Given the "reusable_backgrounds/preparation.feature" passes don't contribute to describing the particular context of a scenario. One way I like to think about scenarios and their steps is whether or not someone with no context of the project would understand the described behavior just from reading the Gherkin. The given step I called out before would likely mean nothing to someone in that case. Additionally, the fact that it's a feature file feels like an incorrect usage of the tool as preparation isn't a project feature that's to be tested.

Again to echo l3pp4rd's comment, one of the recommended ways to handle setup is via helper functions paired with a more declarative Given step.

I think the comments added by mpkorstanje and luke-hill on the following issue are very applicable to your particular use case. Please give it a read and see if you're able to use any of the guidance to improve your setup/glue code.

#335

Thank you again for taking the time to describe your use-case and provide a PR supporting it, however I'm going to close this issue and the PR. If you have any questions, feel free to reach out.

@mxygem mxygem closed this as completed Apr 30, 2021
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 a pull request may close this issue.

3 participants