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

Add new hooks for global level (BeforeTests and AfterTests) that run onl... #672

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
6 participants
@andryutz10
Contributor

andryutz10 commented Feb 23, 2014

Add new hooks for global level (@BeforeAll and @afterall) that run only once before all and after all existing features/scenarios, and new hooks for feature level (@BeforeFeature and @AfterFeature) that run only once before and after a feature(all scenarios in a feature file). Unit test and cucumber test to cover the new hooks and the running order between hooks.
An execution flow with the new hooks looks like this:
1 - @BeforeAll hook(s) (runs before all features once)
2 - @BeforeFeature hook(s) (runs before a feature, without tags or with tags for tagged features)
3 - @before hook(s) (no change, same behavior - before every scenario or tagged scenarios)
4 - @after hook(s) (no change, same behavior - after every scenario or tagged scenarios)
5 - .... 3 and 4 for all scenarios in a feature
6 - @AfterFeature hook(s) (runs after a feature without tags or with tags for tagged features)
7 - ....2 to 6 for every feature
8 - @afterall hook(s) (runs once after all the features are run)

As test for the new hooks see/run:

  • cucumber.runtime.java.hooks.RunWithAllHooksTest
  • cucumber.runtime.java.hooks.HooksStepDefs
  • test\resources\cucumber\runtime\java\hooks\feature-1.feature
  • test\resources\cucumber\runtime\java\hooks\feature-2.feature

Added junit tests and cucumber tests for new code

Add new hooks for global level (BeforeTests and AfterTests) that run …
…only once before all and after all existing features/scenarios, and new hooks for feature level (BeforeFeature and AfterFeature) that run only one before and after a feature(all scenarios in a feature file). Unit test and cucumber test to cover the new hooks and the running order between hooks.
@dkowis

This comment has been minimized.

Member

dkowis commented Feb 25, 2014

This seems tested and proved that this works, so I'm okay with merging it. This is something people have asked for reasonably often, so @andryutz10 thanks for doing the work!

However, something about this bothers me a little bit. Ah, so BeforeAll is now BeforeTests, I don't care for that name very much, I prefer Before/After All, as that immediately makes me think before/after the whole thing. Before/After Test makes me think of Junit. (Somewhat difficult to articulate.)

It also seems like there are a Whole Lot tm of hooks now, and that there should be a better way of handling injection into the workflow. But I'm not sure what it is, I just see a lot of very similar code that's doing very similar things at different places.

LGTM

@mikquinlan

This comment has been minimized.

mikquinlan commented Feb 25, 2014

It may be worth sticking with BDD nomenclature and calling them
@BeforeFeatures and @AfterFeatures.

Then it's obvious what is going on.

On 24 February 2014 19:27, David Kowis notifications@github.com wrote:

This seems tested and proved that this works, so I'm okay with merging it.
This is something people have asked for reasonably often, so @andryutz10https://github.com/andryutz10thanks for doing the work!

However, something about this bothers me a little bit. Ah, so BeforeAllis now
BeforeTests, I don't care for that name very much, I prefer Before/After
All, as that immediately makes me think before/after the whole thing.
Before/After Test makes me think of Junit. (Somewhat difficult to
articulate.)

It also seems like there are a Whole Lot tm of hooks now, and that there
should be a better way of handling injection into the workflow. But I'm not
sure what it is, I just see a lot of very similar code that's doing very
similar things at different places.

[image: LGTM] http://www.lgtm.in/i/oRj2


Reply to this email directly or view it on GitHubhttps://github.com//pull/672#issuecomment-35971341
.

@brasmusson

This comment has been minimized.

Contributor

brasmusson commented Feb 25, 2014

There are two sets of hooks added in the PR

  • @Before/AfterAllTest/Features (before/after everything)
  • @Before/AfterFeature (before/after each feature)

They should probably be handled in separate RP:s.

@andryutz10

This comment has been minimized.

Contributor

andryutz10 commented Feb 25, 2014

Thanks guys for your time and your suggestions.
So just to clarify, I'll rename BeforeTests>BeforeAll and AfterTests>AfterAll.
@mikquinlan, I like your suggestion, but I think that it might be confusing with the other two (BeforeFeature and AfterFeature), because then we'll have BeforeFeature/BeforeFeatures and AfterFeature/AfterFeatures, with the autocomplete in the IDE you might get the wrong one because they are very similar (just a 's' at the end).
If it's ok with you guys I'll just add another commit with the rename of BeforeTests to BeforeAll and AfterTests to AfterAll.

@andryutz10

This comment has been minimized.

Contributor

andryutz10 commented Feb 28, 2014

@dkowis, @mikquinlan I added a new commit to this pull request for the renaming you suggested.
@brasmusson from what I understood there will be now parallel execution until version 3 of Gherkin will be used, so I don't see it as a bad idea. As for parallel execution I think there will be more serious issues to with the current model of execution, but as I seen in some of the comments for parallel executions the execution model will change with Gherkin 3. Another issue with parallel executions will be with the guys that will have to write cucumber feature/scenarios that will be executed in parallel, because they'll have to take care of the states of global objects (eg. :queues, database etc...) used for tests, which is not easy job for a QA for example, but that's only my opinion and I might be wrong.

@mikquinlan

This comment has been minimized.

mikquinlan commented Feb 28, 2014

Cool.

I might be missing something here, but does this not mean that we now have:

@Before/AfterAll - before/after all features are/have been run
@Before/AfterFeature - before/after each feature
@Before/After - before/after each scenario

So should @Before/After be renamed to @BeforeScenario/AfterScenario so that the intension of @Before/After is not lost?

I'm happy to keep as is, just wanted to get that out there so we're clear.

Thanks

@aslakhellesoy

This comment has been minimized.

Contributor

aslakhellesoy commented Feb 28, 2014

What @brasmusson said. Please create separate PRs for @BeforeAll/@AfterAll and @BeforeFeature/@AfterFeature.

The former is much more likely to be accepted than the latter. We'll have to discuss that one with the maintainers of the other Cucumber implementations. Personally I'm sceptical about @BeforeFeature/@AfterFeature, but we'll see what other say.

@andryutz10

This comment has been minimized.

Contributor

andryutz10 commented Feb 28, 2014

@mikquinlan, yes, that is what we'll have.
As for @Before/After, I would like to renamed to @BeforeScenario/AfterScenario, but that will kind of break the compatibility. If this is not a problem I'm happy to rename them.
@aslakhellesoy, I would put them in separate PR, even though I'll lose the tests for using both in the same time.

@dkowis

This comment has been minimized.

Member

dkowis commented Feb 28, 2014

Tests for using both at the same time can be added when both exist. I wouldn't sweat that yet :)

@aslakhellesoy

This comment has been minimized.

Contributor

aslakhellesoy commented Feb 28, 2014

Please keep PRs as minimal as possible. Then less things can go wrong in the approval process ;-)

@mattwynne

This comment has been minimized.

Member

mattwynne commented Mar 5, 2014

I genuinely appreciate your contribution @andryutz10, but I'm giving this the 👎

BeforeFeature / AfterFeature is an anti-pattern. See my comments in #515 about whether we really need BeforeAll / AfterAll anyway.

@andryutz10

This comment has been minimized.

Contributor

andryutz10 commented Mar 5, 2014

@mattwynne, I agree with you on BeforeFeature/AfterFeature, as for the BeforeAll/AfterAll see my comments on #515, thats the reason why I split them in two different PR (see #678).
But sure if you think that there is no need for them I'll just drop the PRs.

@aslakhellesoy

This comment has been minimized.

Contributor

aslakhellesoy commented Jun 26, 2014

Closing this. We'll only add BeforeAll and AfterAll.

@lock

This comment has been minimized.

lock bot commented Oct 25, 2018

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Oct 25, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.