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

Align method and field search algorithms with Java semantics #3553

Closed
5 tasks done
sbrannen opened this issue Nov 10, 2023 · 4 comments
Closed
5 tasks done

Align method and field search algorithms with Java semantics #3553

sbrannen opened this issue Nov 10, 2023 · 4 comments

Comments

@sbrannen
Copy link
Member

sbrannen commented Nov 10, 2023

Overview

As discussed in #3532 (comment), the search algorithms in JUnit 5 that are used to locate methods and fields have historically had the following undesirable characteristic.

A field supersedes (shadows, hides, overrides) a field with the same name in a superclass unless a filter (Predicate) is applied that ignores one of the fields (such as a static check or a check for the presence of a specific annotation).

This also applies to any methods looked up via JUnit 5's reflection support -- for example, lifecycle methods and test methods in JUnit Jupiter.

The JUnit 5 team discussed this topic today and decided to revise our search algorithms to align with Java semantics regarding visibility and overriding.

Specifically, we want to ensure that our search algorithms do not treat something as overridden when it is not overridden according to the Java Virtual Machine Specification (JVMS).

As a consequence, several currently unsupported use cases would become supported -- for example:

  • Two @TempDir fields with the same name in a superclass and subclass would both be injected.
  • Two @BeforeEach methods with the same name/signature in a superclass and subclass would both be invoked, if the @BeforeEach method in the superclass is package-private and the subclass resides in a different package (i.e., the @BeforeEach method in the subclass does not @Override the @BeforeEach method in the superclass according to the JVMS).

External Resources

Related Issues

Deliverables

  • Align method and field search algorithms with Java semantics regarding visibility/overridability.
  • Introduce tests for overridden method support. See comment below.
  • Introduce a configuration parameter / system property -- for example, junit.platform.reflection.search.useLegacySemantics = true -- that allows a project to revert to the previous behavior.
  • Document in User Guide.
  • Document in Release Notes.
@sbrannen sbrannen self-assigned this Nov 10, 2023
@sbrannen sbrannen changed the title Align method and field search algorithms with Java semantics regarding visibility Align method and field search algorithms with Java semantics Nov 10, 2023
@sbrannen

This comment was marked as outdated.

@sbrannen sbrannen added this to the 5.11 M1 milestone Nov 10, 2023
sbrannen added a commit to sbrannen/junit5 that referenced this issue Jan 7, 2024
This is a work in progress.

Currently based on main (5.11 M1 snapshots) and currently only applied
to our method search algorithm.

Aside from the renaming of isMethodShadowedByLocalMethods(), this
commit should no longer contain changes to
ReflectionUtils.findAllMethodsInHierarchy() once 64ed21a has been
reverted.

See junit-team#3553
@sbrannen
Copy link
Member Author

sbrannen commented Jan 7, 2024

Current work on this issue can be viewed in the following feature branch.

main...sbrannen:junit5:issues/3553-java-visibility-semantics-for-methods-and-fields

sbrannen added a commit that referenced this issue Jan 15, 2024
This commit reverts the functional changes from commit f30a8d5 and
disables the associated tests for the time being.

See #3532
See #3553
Closes #3638
sbrannen added a commit that referenced this issue Jan 15, 2024
This commit reverts the functional changes from commit f30a8d5 and
disables the associated tests for the time being.

See #3532
See #3553
Closes #3638

(cherry picked from commit 7789d32)
sbrannen added a commit that referenced this issue Jan 15, 2024
sbrannen added a commit that referenced this issue Jan 15, 2024
…lgorithms"

This reverts commit a670d10.

See #3534
See #3553
See #3600

(cherry picked from commit 2f05a7c)
sbrannen added a commit that referenced this issue Jan 15, 2024
This commit reverts the functional changes from commit 64ed21a and
disables the associated tests for the time being.

See #3498
See #3500
See #3553
Closes #3600
sbrannen added a commit that referenced this issue Jan 15, 2024
This commit reverts the functional changes from commit 64ed21a and
disables the associated tests for the time being.

See #3498
See #3500
See #3553
Closes #3600

(cherry picked from commit c2f49f6)
@sbrannen

This comment was marked as outdated.

sbrannen added a commit to sbrannen/junit5 that referenced this issue Feb 4, 2024
This is a work in progress.

TODO

- Introduce JVM system property to switch to legacy search mode.

- Update all documentation that mentions hiding/superseding semantics.
  - User Guide and Javadoc.

- Document in Release Notes.

See junit-team#3553
@sbrannen

This comment was marked as outdated.

sbrannen added a commit to sbrannen/junit5 that referenced this issue Feb 28, 2024
This is a work in progress.

TODO

- Introduce JVM system property to switch to legacy search mode.

- Update all documentation that mentions hiding/superseding semantics.
  - User Guide and Javadoc.

- Document in Release Notes.

See junit-team#3553
sbrannen added a commit to sbrannen/junit5 that referenced this issue Feb 28, 2024
sbrannen added a commit to sbrannen/junit5 that referenced this issue Feb 28, 2024
sbrannen added a commit to sbrannen/junit5 that referenced this issue Mar 17, 2024
This is a work in progress.

TODO

- Introduce JVM system property to switch to legacy search mode.

- Update all documentation that mentions hiding/superseding semantics.
  - User Guide and Javadoc.

- Document in Release Notes.

See junit-team#3553
sbrannen added a commit to sbrannen/junit5 that referenced this issue Mar 17, 2024
sbrannen added a commit to sbrannen/junit5 that referenced this issue Mar 17, 2024
I'm intentionally leaving this commit in the Git history, in case we
later decide to re-introduce these tests.

See junit-team#3553
sbrannen added a commit to sbrannen/junit5 that referenced this issue Mar 17, 2024
sbrannen added a commit to sbrannen/junit5 that referenced this issue Mar 21, 2024
This is a work in progress.

TODO

- Introduce JVM system property to switch to legacy search mode.

- Update all documentation that mentions hiding/superseding semantics.
  - User Guide and Javadoc.

- Document in Release Notes.

See junit-team#3553
sbrannen added a commit to sbrannen/junit5 that referenced this issue Mar 21, 2024
sbrannen added a commit to sbrannen/junit5 that referenced this issue Mar 21, 2024
I'm intentionally leaving this commit in the Git history, in case we
later decide to re-introduce these tests.

See junit-team#3553
sbrannen added a commit to sbrannen/junit5 that referenced this issue Mar 21, 2024
sbrannen added a commit to sbrannen/junit5 that referenced this issue Apr 6, 2024
This is a work in progress.

TODO

- Introduce JVM system property to switch to legacy search mode.

- Update all documentation that mentions hiding/superseding semantics.
  - User Guide and Javadoc.

- Document in Release Notes.

See junit-team#3553
sbrannen added a commit to sbrannen/junit5 that referenced this issue Apr 6, 2024
sbrannen added a commit to sbrannen/junit5 that referenced this issue Apr 6, 2024
I'm intentionally leaving this commit in the Git history, in case we
later decide to re-introduce these tests.

See junit-team#3553
sbrannen added a commit to sbrannen/junit5 that referenced this issue Apr 6, 2024
sbrannen added a commit to sbrannen/junit5 that referenced this issue Apr 6, 2024
sbrannen added a commit to sbrannen/junit5 that referenced this issue Apr 6, 2024
sbrannen added a commit to sbrannen/junit5 that referenced this issue Apr 6, 2024
I'm intentionally leaving this commit in the Git history, in case we
later decide to re-introduce these tests.

See junit-team#3553
sbrannen added a commit to sbrannen/junit5 that referenced this issue Apr 6, 2024
sbrannen added a commit that referenced this issue Apr 7, 2024
This commit modifies the method and field search algorithms in
ReflectionUtils in order to align with standard Java semantics
regarding whether a given field or method is visible or overridden
according to the rules of the Java language.

Specifically, this commit:

- Renames isMethodShadowedBy() to isMethodOverriddenBy().

- Revises the logic in isMethodOverriddenBy() to determine if one
  method overrides another method according to the rules of the Java
  language.

- Revises isFieldShadowedByLocalFields() to always return false for the
  time being.

- Revises existing tests accordingly; introduces new tests for the new
  behavior; and disables existing tests for the legacy behavior.

See #3553
sbrannen added a commit that referenced this issue Apr 7, 2024
This commit introduces support for the following JVM system property
which can be set to "true" to revert to legacy search semantics
for fields and methods.

junit.platform.reflection.search.useLegacySemantics

See #3553
sbrannen added a commit that referenced this issue Apr 7, 2024
I'm intentionally leaving this commit in the Git history, in case we
later decide to re-introduce these tests.

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