WICKET-7170: AnnotProxyFieldValueFactory now considers defaultCandida…

[hosea]

…te-Flag

[hosea]

Fix for https://issues.apache.org/jira/browse/WICKET-7170

[martin-g]

Why several test classes are removed ?

[hosea]

Why several test classes are removed ?

Those classes were beans and interfaces only used by AnnotProxyFieldValueFactoryTest, so I inlined them as inner classes in the test.
And then cleaned up, for example: InjectableInterface and Bean2 had no benefit. Please have a look at the tests: they go much deeper in detail and are more fine-grained now, but it was a complete refactoring of the tests. This makes the diff hard to read.

[martin-g]

so I inlined them as inner classes

This makes the diff bigger and harder to review. I'd suggest to undo those changes.

[hosea]

Hi @martin-g ,
I do not agree. These classes were designed only for being used inside AnnotProxyFieldValueFactoryTest.

A simple class, just to define a type without any properties and methods for exactly on testclass like

static class Bean{
}

should always be a inner class of the testclass.

And have a look at the classes SpringBeanInjectable and the (same-structured) JakartaInjectInjectable:

public static class SpringBeanInjectable {
	private Bean nobean;

	@SpringBean
	private Bean beanByClass;

	@SpringBean(name = "somebean")
	private Bean beanByName;

	@SpringBean(required = false)
	private Bean optional;

	@Override
	public String toString() {
		return "SpringBeanInjectable";
	}
}

It is exactly build for AnnotProxyFieldValueFactoryTest. From AnnotProxyFieldValueFactoryTest:

obj.getClass().getDeclaredField("nobean");
obj.getClass().getDeclaredField("beanByClass");
obj.getClass().getDeclaredField("beanByName");
springBeanInjectable.getClass().getDeclaredField("optional");

So this classes should also be inner classes in my opinion.
The Marker-Interface Injectable and the testclasses JakartaInjectAnnotProxyFieldValueFactoryTest and SpringBeanAnnotProxyFieldValueFactoryTest could be removed, because I integrated using @ParameterizedTest (test Jakarta-Inject and Spring-Bean-Inject with same @ParameterizedTest)

Just not: The real diff in implementation ist not very much. This is just adding one functionality and a small refactoring:

1. move the "isPrimary"-loop out in a a loop with a Predicate
2. move the "isFieldname"-Rule out in a method
3. Add implementation for "isDefaultCandidate"

But the test is massivley improved:
I replaced the 3 small and not very detailed methods "testFactory", "testCache" and "testFailsIfBeanWithIdIsNotFound" with a bunch of methods testing a lot a scenarios:

• shouldCreateProxyForBeanName
• shouldThrowExceptionIfBeanNameNotFound
• shouldCreateProxyForClass
• shouldThrowException_beanNameAmbiguous
• shouldCreateProxyForUniquePrimary_beanNameAmbiguous
• shouldCreateProxyForUniqueDefaultCandidate_beanNameAmbiguous
• shouldCreateProxyForFieldname_beanNameAmbiguous
• shouldIgnoreUnannotatedFields
• required
• optional
• testCacheForClass
• testCacheForBeanName

Kind regards
Hans

[martin-g]

Well, my experience is the opposite: restructuring and bugfix/feature impl should not be mixed in the same Pull Request! Even worse in the same commit!

This way the bugfix/feature is hidden in a lot of noise (the restructuring).
It is not clear whether the bugfix/feature does not break some previous behavior because now the old tests are reworked too. Even though the intention was to improve them there is a chance that they were actually broken.

I am grateful for your contribution but I won't review it in its current state.
Hopefully someone else from the team will do it.

[hosea]

I understand your point. I will split it into 2 PRs.

[martin-g]

Thank you, @hosea !

[hosea]

Follow-up PR is #1311:
Just implementation/bugfix + additional TestClass for the implementation/bugfix.
No existing running tests touched.