GeezerVison

main changes look OK, I've added some suggestions to reduce the duplication a bit.

I've left some comments about the test changes. I am not a fan of changing existing tests, why are we doing that? Why don't we write dedicated tests for this?

Not changing the existing test will make it less visible what is being tested by the test.
The current test class ReferencedAccessibilityTest.java should test both target and source messages, otherwise the test class is incorrectly named in my opinion.

I can split it to be more precise, so that we get separate tests for source and target. This will be obvious in the classes then.
These three categories will be visible then:

• No messages
• Target messages
• Source messages

main changes look OK, I've added some suggestions to reduce the duplication a bit.
I've left some comments about the test changes. I am not a fan of changing existing tests, why are we doing that? Why don't we write dedicated tests for this?

Not changing the existing test will make it less visible what is being tested by the test. The current test class ReferencedAccessibilityTest.java should test both target and source messages, otherwise the test class is incorrectly named in my opinion.

I can split it to be more precise, so that we get separate tests for source and target. This will be obvious in the classes then. These three categories will be visible then:

• No messages
• Target messages
• Source messages

Took another look at the code. and I think I'll alter the tests still, but do it in another way entirely.
I'm going to introduce tests for the target/source messages, and remove the current messages out of the current tests, because that does not seem to be what the test is actually about, but a side-effect which is added to those tests.

Ideally we should not touch the existing tests and add new ones for this. Or update the ones that are really testing the error messages. The ReferencedAccessibilityTest doesn't sound like the right one.

The DottedErrorMessageTest might be more correct for this. Perhaps even the two UnmappedSourceTest

Ideally we should not touch the existing tests and add new ones for this. Or update the ones that are really testing the error messages. The ReferencedAccessibilityTest doesn't sound like the right one.

The DottedErrorMessageTest might be more correct for this. Perhaps even the two UnmappedSourceTest

Indeed the DottedErrorMessageTest does sound like the correct location. I didn't find that one earlier, because the text Mapping from property is not present in that test, instead it has it in a static variable Mapping from "+ PROPERTY.

I do feel like changing ReferencedAccessibilityTest so that the message is not part of it anymore is the right call, because it is using a side-effect to test behavior while that should be explicitly tested.
(I could do that in a separate PR though, easy enough to separate.)

I do feel like changing ReferencedAccessibilityTest so that the message is not part of it anymore is the right call, because it is using a side-effect to test behavior while that should be explicitly tested.

I am not sure that I am following. What is wrong with that test? The test is properly showing that private methods are not going to be picked up by MapStruct and thus lead to the default warning.

I do feel like changing ReferencedAccessibilityTest so that the message is not part of it anymore is the right call, because it is using a side-effect to test behavior while that should be explicitly tested.

I am not sure that I am following. What is wrong with that test? The test is properly showing that private methods are not going to be picked up by MapStruct and thus lead to the default warning.

default settings is in my opinion not something that the test should know of. If at some point in time the default setting changes, a lot of tests need to be fixed because they assumed that default settings would never change.

But it is something that can be discussed outside of this PR.

default settings is in my opinion not something that the test should know of. If at some point in time the default setting changes, a lot of tests need to be fixed because they assumed that default settings would never change.

We can agree to disagree about this. In my opinion it is totally OK for a lot of tests to fail if we change a default, this also means that users will be affected by this and it is a good signal that something is not OK.
In any case I do agree we can discuss this outside of this PR. Let me know when you are done with this so that I can give it another pass.

I've accepted your suggestions for the production code, and rewritten the tests to be a part of DottedErrorMessageTest.
This did result in changing the target tests there, because otherwise they would be hard to distinguish from eachother.

Should be good for another pass.

This did result in changing the target tests there, because otherwise they

Yea those are fine since those are around testing the messages we are adapting now. Will merge once CI is green