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

@TempDir field in super class skipped when it has same name as a @TempDir field in subclass #3532

Closed
1 task done
sbrannen opened this issue Nov 3, 2023 · 6 comments
Closed
1 task done

Comments

@sbrannen
Copy link
Member

sbrannen commented Nov 3, 2023

Overview

After fixing #3498, I realized that the same types of bugs exist for finding fields in a type hierarchy.

The fix for fields should be analogous to the fix for methods: apply field predicate before searching type hierarchy.

Example

SuperclassTempDirTests passes, but SubclassTempDirTests fails unless you rename one of the @TempDir fields to something other than tempDir.

package demo.a;

import static org.assertj.core.api.Assertions.assertThat;

import java.nio.file.Path;

import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.io.TempDir;

public class SuperclassTempDirTests {

	@TempDir
	static Path tempDir;

	protected static Path getStaticTempDir() {
		return tempDir;
	}

	@Test
	void superTest() {
		assertThat(getStaticTempDir()).exists();
	}

}
package demo.b;

import static org.assertj.core.api.Assertions.assertThat;

import java.nio.file.Path;

import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.io.TempDir;

import demo.a.SuperclassTempDirTests;

class SubclassTempDirTests extends SuperclassTempDirTests {

	@TempDir
	Path tempDir;

	Path getInstanceTempDir() {
		return this.tempDir;
	}

	@Test
	void subTest() {
		assertThat(getInstanceTempDir()).exists();
		assertThat(getStaticTempDir()).exists();
	}

}

Related Issues

Deliverables

  • Apply field predicate before searching type hierarchy.
@sbrannen
Copy link
Member Author

sbrannen commented Nov 3, 2023

Tentatively slated for inclusion in 5.10.1; however, if we don't have time to fix it before the release, we may decide to postpone the fix for a subsequent 5.10.x or the 5.11.0 release.

@marcphilipp, I've already submitted PR #3533 (which contains some TODOs), but if I don't complete it before you release 5.10.1 perhaps you can take it over.

sbrannen added a commit to sbrannen/junit5 that referenced this issue Nov 3, 2023
This commit includes a fix with two simple test classes that
demonstrate the issue.

TODO:

- add "formal" tests in ReflectionUtilsTests and AnnotationUtilsTests
- add release note entries

See junit-team#3498
Closes junit-team#3532
@sbrannen sbrannen linked a pull request Nov 3, 2023 that will close this issue
3 tasks
@sbrannen
Copy link
Member Author

sbrannen commented Nov 3, 2023

sbrannen added a commit to sbrannen/junit5 that referenced this issue Nov 4, 2023
Prior to this commit, findFields() and streamFields() in
ReflectionSupport as well as findAnnotatedFields() and
findAnnotatedFieldValues() in AnnotationSupport first searched for all
fields in the type hierarchy and then applied the user-supplied
predicate (or "is annotated?" predicate) afterwards.

This resulted in fields in subclasses incorrectly "shadowing"
package-private fields in superclasses (in a different package) even if
the predicate would otherwise exclude the field in such a subclass.

For example, given a superclass that declares a package-private static
@⁠TempDir "tempDir" field and a subclass (in a different package) that
declares a @⁠TempDir "tempDir" field, when JUnit Jupiter looked up
@⁠TempDir fields for the subclass, the @⁠TempDir "tempDir" field in the
superclass was not found because the @⁠TempDir "tempDir" field shadowed
it based solely on the field signature, ignoring the type of annotation
sought.

To address that, this commit modifies the internal search algorithms in
ReflectionUtils so that field predicates are applied while searching
the hierarchy for fields.

TODO:

- add release note entries

See junit-team#3498
Closes junit-team#3532
sbrannen added a commit to sbrannen/junit5 that referenced this issue Nov 4, 2023
Prior to this commit, findFields() and streamFields() in
ReflectionSupport as well as findAnnotatedFields() and
findAnnotatedFieldValues() in AnnotationSupport first searched for all
fields in the type hierarchy and then applied the user-supplied
predicate (or "is annotated?" predicate) afterwards.

This resulted in fields in subclasses incorrectly "shadowing"
package-private fields in superclasses (in a different package) even if
the predicate would otherwise exclude the field in such a subclass.

For example, given a superclass that declares a package-private static
@⁠TempDir "tempDir" field and a subclass (in a different package) that
declares a @⁠TempDir "tempDir" field, when JUnit Jupiter looked up
@⁠TempDir fields for the subclass, the @⁠TempDir "tempDir" field in the
superclass was not found because the @⁠TempDir "tempDir" field shadowed
it based solely on the field signature, ignoring the type of annotation
sought.

To address that, this commit modifies the internal search algorithms in
ReflectionUtils so that field predicates are applied while searching
the hierarchy for fields.

TODO:

- add release note entries

See junit-team#3498
Closes junit-team#3532
sbrannen added a commit to sbrannen/junit5 that referenced this issue Nov 4, 2023
Prior to this commit, findFields() and streamFields() in
ReflectionSupport as well as findAnnotatedFields() and
findAnnotatedFieldValues() in AnnotationSupport first searched for all
fields in the type hierarchy and then applied the user-supplied
predicate (or "is annotated?" predicate) afterwards.

This resulted in fields in subclasses incorrectly "shadowing"
package-private fields in superclasses (in a different package) even if
the predicate would otherwise exclude the field in such a subclass.

For example, given a superclass that declares a package-private static
@⁠TempDir "tempDir" field and a subclass (in a different package) that
declares a @⁠TempDir "tempDir" field, when JUnit Jupiter looked up
@⁠TempDir fields for the subclass, the @⁠TempDir "tempDir" field in the
superclass was not found because the @⁠TempDir "tempDir" field shadowed
it based solely on the field signature, ignoring the type of annotation
sought.

To address that, this commit modifies the internal search algorithms in
ReflectionUtils so that field predicates are applied while searching
the hierarchy for fields.

See junit-team#3498
Closes junit-team#3532
Closes junit-team#3533
sbrannen added a commit that referenced this issue Nov 4, 2023
Prior to this commit, findFields() and streamFields() in
ReflectionSupport as well as findAnnotatedFields() and
findAnnotatedFieldValues() in AnnotationSupport first searched for all
fields in the type hierarchy and then applied the user-supplied
predicate (or "is annotated?" predicate) afterwards.

This resulted in fields in subclasses incorrectly "shadowing"
package-private fields in superclasses (in a different package) even if
the predicate would otherwise exclude the field in such a subclass.

For example, given a superclass that declares a package-private static
@⁠TempDir "tempDir" field and a subclass (in a different package) that
declares a @⁠TempDir "tempDir" field, when JUnit Jupiter looked up
@⁠TempDir fields for the subclass, the @⁠TempDir "tempDir" field in the
superclass was not found because the @⁠TempDir "tempDir" field shadowed
it based solely on the field signature, ignoring the type of annotation
sought.

To address that, this commit modifies the internal search algorithms in
ReflectionUtils so that field predicates are applied while searching
the hierarchy for fields.

See #3498
Closes #3532
Closes #3533

(cherry picked from commit f30a8d5)
sbrannen added a commit to sbrannen/junit5 that referenced this issue Nov 4, 2023
This commit consistently applies method predicates in search algorithms
analogous to the application of field predicates.

See junit-team#3498
See junit-team#3532
Closes junit-team#3534
sbrannen added a commit to sbrannen/junit5 that referenced this issue Nov 4, 2023
This commit consistently applies method predicates in search algorithms
analogous to the application of field predicates.

See junit-team#3498
See junit-team#3532
Closes junit-team#3534
sbrannen added a commit that referenced this issue Nov 4, 2023
This commit consistently applies method predicates in search algorithms
analogous to the application of field predicates.

See #3498
See #3532
Closes #3534
sbrannen added a commit that referenced this issue Nov 4, 2023
This commit consistently applies method predicates in search algorithms
analogous to the application of field predicates.

See #3498
See #3532
Closes #3534

(cherry picked from commit a670d10)
@sbrannen
Copy link
Member Author

sbrannen commented Nov 4, 2023

This has been merged into main and backported to 5.10.x.

@Marcono1234
Copy link
Contributor

Marcono1234 commented Nov 6, 2023

Out of curiosity (not because my tests currently do this), this only covers the case where one field is static and the other one is non-static, right?
Adjusting your example so that either both fields are static or both are non-static seems to still fail in 5.10.1.

@sbrannen
Copy link
Member Author

sbrannen commented Nov 7, 2023

Hi @Marcono1234,

Out of curiosity (not because my tests currently do this), this only covers the case where one field is static and the other one is non-static, right? Adjusting your example so that either both fields are static or both are non-static seems to still fail in 5.10.1.

Yes, that's correct. A field supersedes a package private field with the same name in a superclass when the superclass resides in a different package 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).

That's the status quo, albeit perhaps not very intuitive.

This also applies to lifecycle and test methods (and well, for any methods looked up via JUnit 5's reflection support).

We've had numerous discussions about this behavior over the years, and I think it might be worth discussing again within the team.

In light of that, I plan to create a new issue to revisit the topic.

Thanks for bringing it up! 👍


Related Issues

(so that I don't lose track of them)

@sbrannen
Copy link
Member Author

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)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants