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

Are @BeforeEach/All the most unambiguous names? #163

Closed
nicolaiparlog opened this Issue Feb 23, 2016 · 18 comments

Comments

Projects
None yet
@nicolaiparlog
Contributor

nicolaiparlog commented Feb 23, 2016

Yesterday, at Objektforum, @jlink invited us to bike-shed so here we go. :)

I think @BeforeAll and @BeforeEach are somewhat ambiguous. While grammar is on your side, the difference in detail might be overlooked by many developers, the vast majority of which speak English as a second language.

This might be aggravated when only one of the annotations is used. Without the juxtaposition "all vs each" a developer might have to look up when the annotation triggers the method's execution or, worse, assume the wrong interpretation.

My proposal is @BeforeFirst,@BeforeEach (and @AfterEach, @AfterLast). I generally like them but I see the drawback that they start talking about order, which JUnit eschews on purpose and with success.

@schauder

This comment has been minimized.

Show comment
Hide comment
@schauder

schauder Feb 23, 2016

Contributor

Took me some time time to see the ambiguity. Maybe because I'm a long time user (like probably all of those discussing here), but I find @BeforeFirst even more amiguous. What if "first" doesn't get executed, because it gets skipped? Or we execute only a single testmethod out of N ... The name @BeforeFirst seem to suggest it wouldn't get executed in these cases.

Contributor

schauder commented Feb 23, 2016

Took me some time time to see the ambiguity. Maybe because I'm a long time user (like probably all of those discussing here), but I find @BeforeFirst even more amiguous. What if "first" doesn't get executed, because it gets skipped? Or we execute only a single testmethod out of N ... The name @BeforeFirst seem to suggest it wouldn't get executed in these cases.

@nicolaiparlog

This comment has been minimized.

Show comment
Hide comment
@nicolaiparlog

nicolaiparlog Feb 23, 2016

Contributor

Good points against first/last. But would you agree (tentatively at least) that developers might not easily spot the difference between all/each? I'm sure someone else will come up with a better alternative if need be.

Btw, there are other testing frameworks using the exact same words (Jasmine, RSpec) so there is precedence.

Contributor

nicolaiparlog commented Feb 23, 2016

Good points against first/last. But would you agree (tentatively at least) that developers might not easily spot the difference between all/each? I'm sure someone else will come up with a better alternative if need be.

Btw, there are other testing frameworks using the exact same words (Jasmine, RSpec) so there is precedence.

@schauder

This comment has been minimized.

Show comment
Hide comment
@schauder

schauder Feb 23, 2016

Contributor

I have no idea if anybody ever had a problem with the meaning of Each/All in this context. I most certainly never had, and I never noticed anybody having that problem.

Of course that might be, because most of the time where this could come up, is when I explain how this kind of stuff works and then I explain both in short succession, so the potential confusion is already fixed before it could surface.

Contributor

schauder commented Feb 23, 2016

I have no idea if anybody ever had a problem with the meaning of Each/All in this context. I most certainly never had, and I never noticed anybody having that problem.

Of course that might be, because most of the time where this could come up, is when I explain how this kind of stuff works and then I explain both in short succession, so the potential confusion is already fixed before it could surface.

@marcphilipp

This comment has been minimized.

Show comment
Hide comment
@marcphilipp

marcphilipp Feb 29, 2016

Member

I would rather keep Each/All following @schauder's arguments.

@junit-team/junit-lambda What do you think?

Member

marcphilipp commented Feb 29, 2016

I would rather keep Each/All following @schauder's arguments.

@junit-team/junit-lambda What do you think?

@sbrannen

This comment has been minimized.

Show comment
Hide comment
@sbrannen

sbrannen Feb 29, 2016

Member

I am in favor of maintaining the status quo, simply because no one has proposed names that are better suited to the task at hand.

Member

sbrannen commented Feb 29, 2016

I am in favor of maintaining the status quo, simply because no one has proposed names that are better suited to the task at hand.

@ggcrjdev

This comment has been minimized.

Show comment
Hide comment
@ggcrjdev

ggcrjdev Mar 11, 2016

I thought about the annotations names and maybe names like @PreTest/@PostTest rather than @BeforeEach/@AfterEach and @PreBundle/@postbundle rather than @BeforeAll/@afterall could be better.
Note: The term bundle I use to represent a set of ests in a class or package.

I thought about the annotations names and maybe names like @PreTest/@PostTest rather than @BeforeEach/@AfterEach and @PreBundle/@postbundle rather than @BeforeAll/@afterall could be better.
Note: The term bundle I use to represent a set of ests in a class or package.

@sbrannen

This comment has been minimized.

Show comment
Hide comment
@sbrannen

sbrannen Mar 11, 2016

Member

We have intentionally avoided names like @PreTest simply because that implies that such a method executes before an @Test method; however, it may be the case that an @BeforeEach method is executed before an @Step method in a scenario test.

Thus, we aim to keep the common annotation names generic in order to avoid an unnecessary proliferation of annotations.

In addition, the term bundle does not seem applicable in a programming model for tests.

Member

sbrannen commented Mar 11, 2016

We have intentionally avoided names like @PreTest simply because that implies that such a method executes before an @Test method; however, it may be the case that an @BeforeEach method is executed before an @Step method in a scenario test.

Thus, we aim to keep the common annotation names generic in order to avoid an unnecessary proliferation of annotations.

In addition, the term bundle does not seem applicable in a programming model for tests.

@jillesvangurp

This comment has been minimized.

Show comment
Hide comment
@jillesvangurp

jillesvangurp Jun 9, 2016

I agree the names are fine. A bigger concern for me are the semantics: when do these things actually run and how? Junit currently requires these methods to be static, which I think is a needless complication inherited from the junit3 days. I recall having a issues with junit 4 a long time ago where we were doing some expensive cleanup in BeforeAll annotated methods where to our surprise the methods executed right after the test suite initialized before any of the tests in any of the test classes had actually run. Since the whole point was doing some expensive database cleanup in between test classes, this sort of got us stuck for a while. This makes for a bit awkward semantics. Junit5 might be a nice opportunity to clean that up.

As I understand it the reasoning for requiring static methods has to do with not allowing people to share state between methods in a unit test. This is valid for unit tests but kind of impractical for scenario or integration tests where you might actually have valid reasons for this. Since the name is changing with junit5 in any case, this might be a good opportunity to drop that requirement.

For reference, TestNg has BeforeSuite, AfterSuite, BeforeClass, AfterClass, BeforeMethod, and AfterMethod annotations that you put on non static methods.

I agree the names are fine. A bigger concern for me are the semantics: when do these things actually run and how? Junit currently requires these methods to be static, which I think is a needless complication inherited from the junit3 days. I recall having a issues with junit 4 a long time ago where we were doing some expensive cleanup in BeforeAll annotated methods where to our surprise the methods executed right after the test suite initialized before any of the tests in any of the test classes had actually run. Since the whole point was doing some expensive database cleanup in between test classes, this sort of got us stuck for a while. This makes for a bit awkward semantics. Junit5 might be a nice opportunity to clean that up.

As I understand it the reasoning for requiring static methods has to do with not allowing people to share state between methods in a unit test. This is valid for unit tests but kind of impractical for scenario or integration tests where you might actually have valid reasons for this. Since the name is changing with junit5 in any case, this might be a good opportunity to drop that requirement.

For reference, TestNg has BeforeSuite, AfterSuite, BeforeClass, AfterClass, BeforeMethod, and AfterMethod annotations that you put on non static methods.

@sbrannen

This comment has been minimized.

Show comment
Hide comment
@sbrannen

sbrannen Jun 9, 2016

Member

@jillesvangurp,

Scenario tests should address your concerns.

Please see the discussion in #48 for details.

Member

sbrannen commented Jun 9, 2016

@jillesvangurp,

Scenario tests should address your concerns.

Please see the discussion in #48 for details.

@jillesvangurp

This comment has been minimized.

Show comment
Hide comment
@jillesvangurp

jillesvangurp Jun 10, 2016

@sbrannen I responded to that issue earlier. To be honest, I don't think the annotations proposed there are anywhere near good enough yet compared to things, especially if BDD style testing is what you want. Frankly, I think it all looks a bit too verbose and too limited to me in the current form. I'm not sure if it is a fixable problem given Java's semantics.

I'm not that much into BDD myself but I do need to write integration tests with sane before and after semantics, which is why I brought this up because I think it is an area where junit can improve.

@sbrannen I responded to that issue earlier. To be honest, I don't think the annotations proposed there are anywhere near good enough yet compared to things, especially if BDD style testing is what you want. Frankly, I think it all looks a bit too verbose and too limited to me in the current form. I'm not sure if it is a fixable problem given Java's semantics.

I'm not that much into BDD myself but I do need to write integration tests with sane before and after semantics, which is why I brought this up because I think it is an area where junit can improve.

@sbrannen

This comment has been minimized.

Show comment
Hide comment
@sbrannen

sbrannen Jun 10, 2016

Member

@jillesvangurp, oops... I didn't reread the comments in #48. So I didn't realize you'd already participated in that discussion.

To be honest, I don't think the annotations proposed there are anywhere near good enough yet compared to things

Compared to what things?

In any case, the decision as to whether or not @BeforeAll and @AfterAll should be able to be applied to non-static methods is not related to this issue. This issue only applies to the names of such annotations. So let's stay on topic here. 😉

If you'd like to discuss scenario tests, please do that in #48.

If you'd like to make a separate proposal to support @BeforeAll and @AfterAll on non-static methods, outside the scope of scenario tests, please create a new issue to address that.

Cheers!

Member

sbrannen commented Jun 10, 2016

@jillesvangurp, oops... I didn't reread the comments in #48. So I didn't realize you'd already participated in that discussion.

To be honest, I don't think the annotations proposed there are anywhere near good enough yet compared to things

Compared to what things?

In any case, the decision as to whether or not @BeforeAll and @AfterAll should be able to be applied to non-static methods is not related to this issue. This issue only applies to the names of such annotations. So let's stay on topic here. 😉

If you'd like to discuss scenario tests, please do that in #48.

If you'd like to make a separate proposal to support @BeforeAll and @AfterAll on non-static methods, outside the scope of scenario tests, please create a new issue to address that.

Cheers!

@littleclay

This comment has been minimized.

Show comment
Hide comment
@littleclay

littleclay Jul 9, 2016

While I get it and haven't been personally tripped up by this, I did recognize the ambiguous nature here. What if you changed @BeforeAll and @AfterAll to something like @First and @Last to capture the idea they run only once at the very beginning and end. I was also thinking of something like @BeforeAny but couldn't think of a good after version that wasn't more ambiguous :-)

While I get it and haven't been personally tripped up by this, I did recognize the ambiguous nature here. What if you changed @BeforeAll and @AfterAll to something like @First and @Last to capture the idea they run only once at the very beginning and end. I was also thinking of something like @BeforeAny but couldn't think of a good after version that wasn't more ambiguous :-)

@jbduncan

This comment has been minimized.

Show comment
Hide comment
@jbduncan

jbduncan Jul 10, 2016

Contributor

I had the idea of replacing (Before|After)All with @BeforeStart and @AfterEnd, but after thinking about it, I realise this may be even more ambiguous than the status quo. After all, what part of the test cycle would 'Start' and 'End' refer to? Before/after each method; each class; or all tests?

What does everyone else think of this idea?

Contributor

jbduncan commented Jul 10, 2016

I had the idea of replacing (Before|After)All with @BeforeStart and @AfterEnd, but after thinking about it, I realise this may be even more ambiguous than the status quo. After all, what part of the test cycle would 'Start' and 'End' refer to? Before/after each method; each class; or all tests?

What does everyone else think of this idea?

@smoyer64

This comment has been minimized.

Show comment
Hide comment
@smoyer64

smoyer64 Jul 10, 2016

Contributor

I was going to make an extension that provided @BeforeSuite and @AfterSuite. I thought those names were descriptive enough when placed compared the the existing annotations.

Contributor

smoyer64 commented Jul 10, 2016

I was going to make an extension that provided @BeforeSuite and @AfterSuite. I thought those names were descriptive enough when placed compared the the existing annotations.

@ondrejlerch

This comment has been minimized.

Show comment
Hide comment
@ondrejlerch

ondrejlerch Aug 11, 2016

I also suggest to use naming like in TestNG: BeforeClass, AfterClass, BeforeMethod and AfterMethod.

In my point of view, that is the most descriptive and least ambiguous.

I am also missing @BeforeSuite and @AfterSuite in Junit 5 and I think that this functionality should be added to Junit 5.

Taken to the extreme, do you want to rename @BeforeSuite and @AfterSuite to @BeforeBeforeAll and @AfterAfterAll to be according to new naming convention? :-)

I also think that there should not be any static restriction on the methods.

Let's make Junit 5 the best java testing framework in all aspects.

I also suggest to use naming like in TestNG: BeforeClass, AfterClass, BeforeMethod and AfterMethod.

In my point of view, that is the most descriptive and least ambiguous.

I am also missing @BeforeSuite and @AfterSuite in Junit 5 and I think that this functionality should be added to Junit 5.

Taken to the extreme, do you want to rename @BeforeSuite and @AfterSuite to @BeforeBeforeAll and @AfterAfterAll to be according to new naming convention? :-)

I also think that there should not be any static restriction on the methods.

Let's make Junit 5 the best java testing framework in all aspects.

@mlevison

This comment has been minimized.

Show comment
Hide comment
@mlevison

mlevison Aug 11, 2016

Something that makes the naming a lot more obvious write a trivial sample test with all the different @Before/@after annotations. Put at @test etc in between, a little effort on the formatting and the output will show elegantly which annotation does what. When I'm running a technical course (ie CSD) I get participants to do this for JUnit 4 and NUnit. I ask them to compare and discuss which model they prefer.

Cheers
Mark

Something that makes the naming a lot more obvious write a trivial sample test with all the different @Before/@after annotations. Put at @test etc in between, a little effort on the formatting and the output will show elegantly which annotation does what. When I'm running a technical course (ie CSD) I get participants to do this for JUnit 4 and NUnit. I ask them to compare and discuss which model they prefer.

Cheers
Mark

@HermanBovens

This comment has been minimized.

Show comment
Hide comment
@HermanBovens

HermanBovens Oct 4, 2017

I found this discussion because I noticed the weird naming of @BeforeAll and @BeforeEach. After I read the docs and learnt what the difference was, I wondered why @BeforeAll wasn't called @BeforeAny instead. It would definitely have helped me (non-native English speaker) understand (without reading the docs) that the method is executed before any of the test methods get executed.

I found this discussion because I noticed the weird naming of @BeforeAll and @BeforeEach. After I read the docs and learnt what the difference was, I wondered why @BeforeAll wasn't called @BeforeAny instead. It would definitely have helped me (non-native English speaker) understand (without reading the docs) that the method is executed before any of the test methods get executed.

@sbrannen

This comment has been minimized.

Show comment
Hide comment
@sbrannen

sbrannen Nov 22, 2017

Member

Hi everybody,

Thanks for the feedback!

In light of the fact that 5.0 GA has already been released, I am now closing this issue since we can't change the names of these annotations now anyway.

Cheers,

Sam

Member

sbrannen commented Nov 22, 2017

Hi everybody,

Thanks for the feedback!

In light of the fact that 5.0 GA has already been released, I am now closing this issue since we can't change the names of these annotations now anyway.

Cheers,

Sam

@sbrannen sbrannen closed this Nov 22, 2017

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