Skip to content

Conversation

@Zegveld
Copy link
Contributor

@Zegveld Zegveld commented Feb 8, 2024

updated the message for unmapped source properties to have the same structure as the unmapped target properties messages.
fixes #2788

…ins information about the forged method.
Copy link
Member

@filiphr filiphr left a comment

Choose a reason for hiding this comment

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

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?

@Zegveld
Copy link
Contributor Author

Zegveld commented Feb 11, 2024

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

@Zegveld
Copy link
Contributor Author

Zegveld commented Feb 11, 2024

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.

@filiphr
Copy link
Member

filiphr commented Feb 11, 2024

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

@Zegveld
Copy link
Contributor Author

Zegveld commented Feb 11, 2024

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.)

@filiphr
Copy link
Member

filiphr commented Feb 11, 2024

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.

@Zegveld
Copy link
Contributor Author

Zegveld commented Feb 11, 2024

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.

…source properties are by default ignored.
@filiphr
Copy link
Member

filiphr commented Feb 11, 2024

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.

@Zegveld
Copy link
Contributor Author

Zegveld commented Feb 11, 2024

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.

@filiphr
Copy link
Member

filiphr commented Feb 11, 2024

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

@filiphr filiphr merged commit bb1cd63 into mapstruct:main Feb 11, 2024
@Zegveld Zegveld deleted the feature/2788 branch February 11, 2024 12:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Improve error messages for auto generated mappings

2 participants