Fix cucumber tests messing with tests of dependants

[bartios]

The cucumber @after hook was messing with the tests in projects depending
on this framework. Because the @after hook is run after every scenario
dependants had to either overwrite the file by including a file with the same name
at the same place or do something equally ugly.

By tagging the hook and the only scenario included in the framework with the
"@framework" tag the hook is only ran for that scenario. Thus the hook is not
ran in tests in projects depending on this one while not influencing the testing in
this project.

fixes #83

[bartios]

I talked to @avandeursen after one of his lectures at the TU Delft about this issue, this is the pull request I promised him.

[jwgmeligmeyling]

This solution is not complete. Disposing the launcher twice is a problem, but now there are step definitions that are available to students, such as Given the user has launched the JPacman GUI, that can launch a UI that will never be disposed. Because step definitions should be organised by domain, and these step definitions are available on classpath, it is even intended that they reuse these step definitions, and doing so would cause unwanted behaviour with this PR.

In my opinion, we should move this Cucumber test case to the jpacman-template, and ask students to extend this boilerplate. Then all the class path quirks will be gone and we will eliminate confusion about where the CucumberTest and features should go (template or framework). We should then remove the Cucumber classes and features from the jpacman-framework, or exclude the test-jar of the jpacman-framework from the jpacman-template and move the required test dependencies to there.

[bartios]

Tagging the hook and scenario does not prevent the students from reusing the step definitions for they are not affected by this at all. They will need to tag the scenario's/cases they implement and use their own (tagged) @after methods but that is the way it should probably be done anyway.

Moving it to jpacman -template is probably the easier solution for the students and certainly less finicky. The fact that the template shouldn't be touched looks logical to me (for it is essentially a local copy of a dependency and only there for the earlier assignments and maybe easy acces to the file structure). The way cucumber uses a lot of globally declared things makes it inherently more difficult to work with when used in dependencies.

Overall I think that a untagged @after cucumber method is probably a bad idea in most situations because the things that should truly always be done in a project are too few to justify it possibly breaking future tests or tests done within dependencies.

[jwgmeligmeyling]

1. Cucumber looks up the step definitions it needs to run a specific scenario
2. From this lookup, a list of required classes is constructed. Using DI this instantiation makes more sense, however, without using DI, Cucumber just instantiates all the classes with Step definitions (what happens here).
3. These classes are instantiated, and their Before methods are ran.
4. The scenario runs
5. The After methods are ran for these classes.

This means that the @Before and @After methods are a contract within the Step definition class between these hooks and the definitions themselves. The step definitions require the state that is required and cleaned up in the @Before and @After methods.

In conclusion,

• If you rely on a specific step definition, you should also run its @Befores and @Afters.
• If you do not run step definitions from this class, the @Before and @Afters will not even be ran.

The problem here is two things:

• There are step definitions that can and should be reuse by students (or otherwise they will encounter duplicate step definitions)
• The step definitions, if reused, require this After method to be invoked.

The only true fix is to move this to the template and provide it as boilerplate to edit, or start using DI.

[jwgmeligmeyling]

Admittedly, I am struck by the bad design of this as well. But annotating the @After and @Before will not solve al start up problems we've seen in the lab, and introduce new issues. And in the nature of Cucumber.

For specifying an alternative before I think the @After is a good mechanism. But to hack around an existing @After, meh.. That is smelly 😉

[bartios]

When I think about cucumber the step definitions are like lego bricks with which you build a house in the feature file. It seems logical to me that building an apartment building with these bricks requires different @Before and @After methods then building a car.
Thus as far as I know the step definitions them self have virtually nothing to do with the hooks, only when assembled the matching @After and @Before can be chosen. That is also why I found it a bit perplexing to learn that those are global and will be ran after every scenario by default.
In short that means that cucumber will open a feature file, choose a scenario, run it's @Before, lookup the steps, execute them and then run the scenario's @After.

What you have written above seems to imply a different understanding of cucumber, if that is the case then I probably have a flawed understanding of cucumber.
If not however then I don't see the problem with reusing steps as they have no bind to the hooks on their own.

[jwgmeligmeyling]

It seems to me that you have a very well understanding of Cucumber, you do however miss my point. The fact that you had to change the feature by adding the @Framework annotation, shows that this is a breaking change. Students that wish to use step definitions made in the framework (Given the user has launched the JPacman GUI for example) should now:

• Add the @Framework annotation to their features, so the corresponding @After method is ran as well, and the launcher is disposed properly.

Or:

• Create an @After method themselves, that cleans up the state that has been introduced in another Step Definitions class.

The intention of the @Before and @After methods is to create and clean up state that is required for that domain. The step definitions may then assume that this state is created and properly cleaned up, eliminating duplicate boilerplate (and ensuring that clean up is called even in case of an error). Do not get me wrong, it is very unfortunate that Cucumbers also runs all @After and @Before methods, even if you do not use any step definition from that class.

Imagine the following step definitions:

class StepDefs {

   File temporaryFolder;

   @Before
   public void setUp() {
      temporaryFolder = createTempDir();
   }

   @Given("^A feature file")
   public void createFile() {
       writeContents(new File(temporaryFolder, "my-feature.feature"));
   }

   @After("bliep"
   public void cleanFolder() {
       deleteDirectory(temporaryFolder)
   }
}

If you now create a story that uses A feature file but does not have the @bliep annotation it will break the idea behind StepDefs and the folder will not be deleted.

This year students should have reused the step definitions from the framework, but failed to grab the idea to use the static accessors to the game instance. This annotation suffers from the very same problem, and is potentially even more obscure to find for students.

[avandeursen]

Thanks @bartios, @jwgmeligmeyling. I will carefully reconsider step definitions in the jpacman-framework and the jpacman-template. Using tags sounds like the way to go. Later I'll make a decision on how to proceed. Thanks again!

[bartios]

I get what you mean now (I think) but his pull request was never intended to be accepted before the end of the course. It would indeed break most of the students current solutions, but it repairs a fundamental flaw in the framework as I think we both agree that there shouldn't be any untagged @Before and @After methods. This of course implies that each scenario should also be tagged.

@avandeursen told me when I talked to him that it was unlikely that this request would be accepted before the end of the year and I forgot to explicitly state that in the pull request.

Also, you say

Students that wish to use step definitions made in the framework (Given the user has launched the JPacman GUI for example) should now:

• Add the @framework annotation to their features, so the corresponding @after method is ran as well, and the launcher is disposed properly

...

But a step (lego brick) used in a scenario does not necessarily need to have any consequence for the @After or @Before as the needs for those are defined by the complete scenario (lego house).

[avandeursen]

Yes. Very much appreciated @bartios -- thanks!

[jwgmeligmeyling]

I actually do not agree that annotating Befores and Afters is a good fix, and that they should be annotated. If the step defenitions could have the same annotation, that would be a good fix. If the annotated before/after replaces/overrides another before/after - then thats useful as well. But in all other cases step definitions will be left without their state being properly initialized by their Before, or properly being cleaned up by their after.

This usage is merely a workaround for a bug in Cucumber (naively invoking all before/after methods) and will introduce new difficulties to the students. For example launching a launcher that won't be dismissed.

[jwgmeligmeyling]

Lets continue the conversation at cucumber/cucumber-jvm#1005

[jwgmeligmeyling]

Small update based on cucumber/cucumber-jvm#1005

I see how the @Before and @After are intended now, and why they are always invoked, and I also see the issues related to not running these hooks only if a step definition is used from that class as well. You should therefore keep the computation in these hooks relatively easy, potentially reusing the result from a previous run. However, sometimes you just have to setup more intensive context for some domains within (some of your) tests.

Currently there are two options:

• Use tags. This requires you to make your stories aware of the implementation of the step definitions and classes, which in my opinion contradicts with the key benefit of writing user stories in the first place.
• Separate features and their step definitions in separate packages, and run the features with a different glue package. This obstructs step definition reuse, because you now organise step definitions by feature, and not domain.

I am really looking for a mechanism that allows me to organise my step definitions by domain and be able to hook on scenarios only if this domain is used. My question is, can we do a better job? And how could we implement this (considering that my current idea - only running the hooks if a step definition from that class is used - is pretty much rejected).

re test classes from project dependencies: those classes should not be on your classpath: projects should not, by convention, export their test classes. See #999

That is another issue not related to this topic 😉 Got that!

This is however all not required, if students reuse the step definitions from the framework. They can do this by:

• Using the Given the user has launched the JPacman GUI from the jpacman-framework
• If they use another statement to construct the board (Given the game is started) they should either internally call StateNavigationSteps#theUserHasLaunchedTheJPacmanGUI() or use the accessors to set the Game and Launcher instances . This als means the following:
  • We should use DI to get an instance of StateNavigationSteps, or make all variables, accessors and step definitions in StateNavigationSteps static, so it can be accessed from other step definition classes.
  • We should explain that this reuse is preferred, and give an example in the lecture how we want to see step definition organization, and how you can reuse the StateNavigationSteps.

(For this they should not overwrite nl.tudelft.jpacman.cucumber.StateNavigationSteps on their classpath).

[avandeursen]

Thanks a lot for the extensive discussion. This has inspired the ultimate solution in #105.

Thanks!!!!