Skip to content

Issues/2914 private lifecycle methods should cause exception #2951

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

Merged

Conversation

mmerdes
Copy link
Contributor

@mmerdes mmerdes commented Jun 21, 2022

Overview

private lifecycle methods now cause an exception similar to the behavior for static methods.

(Not yet completely sure how to precisely define 'our' meaning of overridden/shadowed in this context.)


I hereby agree to the terms of the JUnit Contributor License Agreement.


Definition of Done

Copy link
Member

@sbrannen sbrannen left a comment

Choose a reason for hiding this comment

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

private lifecycle methods now cause an exception similar to the behavior for static methods.

Implementation and tests look good.

(Not yet completely sure how to precisely define 'our' meaning of overridden/shadowed in this context.)

I'll ponder it a bit and get back to you if I come up with something.

@sbrannen
Copy link
Member

(Not yet completely sure how to precisely define 'our' meaning of overridden/shadowed in this context.)

Using a thesaurus I came up with the following candidates.

  • camouflaged
  • cloaked
  • obscured
  • superseded
  • suppressed
  • "given precedence"

@mmerdes
Copy link
Contributor Author

mmerdes commented Jun 30, 2022

(Not yet completely sure how to precisely define 'our' meaning of overridden/shadowed in this context.)

Using a thesaurus I came up with the following candidates.

  • camouflaged
  • cloaked
  • obscured
  • superseded
  • suppressed
  • "given precedence"

Personally, I would prefer 'superseded'. To me, it conveys best what is going on without using the s-word.
Any other opinions?

@sbrannen
Copy link
Member

Using a thesaurus I came up with the following candidates.

  • camouflaged
  • cloaked
  • obscured
  • superseded
  • suppressed
  • "given precedence"

Personally, I would prefer 'superseded'. To me, it conveys best what is going on without using the s-word. Any other opinions?

I think 'superseded' is a good choice.

Copy link
Member

@sbrannen sbrannen left a comment

Choose a reason for hiding this comment

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

Please update the PR to enforce that @BeforeAll and @AfterAll lifecycle methods are not allowed to be private as well.

Copy link
Member

@sbrannen sbrannen left a comment

Choose a reason for hiding this comment

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

I raised a question about superseded interface methods.

Copy link
Member

@sbrannen sbrannen left a comment

Choose a reason for hiding this comment

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

The changes look good!

All that's left is the following bullet point.

@mmerdes
Copy link
Contributor Author

mmerdes commented Jul 2, 2022

The changes look good!

All that's left is the following bullet point.

Yes, should be included in the release notes.
Not so sure about the user guide - after all it is a bug fix and the behaviour is now true to the one specified in the user guide...

@sbrannen
Copy link
Member

sbrannen commented Jul 3, 2022

Yes, should be included in the release notes.

I apologize for prematurely removing the "breaking changes" section for Jupiter in the 5.9 RC1 release notes, but that's where I'd put the note.

That's what we've done in the past, along the lines of "Fixed bug XYZ, though this is potentially a breaking change for some users in that the build will now fail..."

Not so sure about the user guide - after all it is a bug fix and the behaviour is now true to the one specified in the user guide...

I meant more along the lines of updating the wording currently in use.

For example, for @BeforeEach we currently state:

Such methods are inherited unless they are overridden.

Specifically, we need to update the following sections.

@mmerdes mmerdes merged commit 07b4f51 into main Jul 4, 2022
@mmerdes mmerdes deleted the issues/2914-private-lifecycle-methods-should-cause-exception branch July 4, 2022 07:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Private lifecycle methods should cause an exception
3 participants