Conversation
3012ef7 to
28c2894
Compare
hankem
left a comment
There was a problem hiding this comment.
Thanks a lot for your contribution! 💚
I'd like to take a closer look whether we can simplify the rule, but I already wanted to leave one comment on the test. 😉
| @@ -0,0 +1,4 @@ | |||
| package com.tngtech.archunit.library.testclasses.packages.correct.twoimplementationsonetestdir1; | |||
|
|
|||
| public class OnlyOneImplementationHaveTestAndItIsMatchingImplPackageTest { | |||
There was a problem hiding this comment.
For your test to cover the use case of #1367, this should be
| public class OnlyOneImplementationHaveTestAndItIsMatchingImplPackageTest { | |
| class MultipleImplementationsWithOnlyOneTestAndInRightPackageTest { |
to make your new test actually fail with the old implementation. 🙂
In addition: I'd drop the AndInRightPackage part from this name: It's a relevant aspect to have classes with the same simple name in different packages, so: Is it right or is it wrong??
How about SimpleNameThatOccursInSeveralPackages?
There was a problem hiding this comment.
I was reneming classes at the end and forgot about that one 🤦 Thanks for this suggestion - applied!
d494fc0 to
b87bd2f
Compare
I have a feeling it might be done in with a simpler check. Sorry for not persuing that before I have created this PR, but I am just before my vacation and had a little time to create the change 🌴 I have another, simpler draft of such a condition, but it requires a bit sloppy name creation function: and then: having: I am not super happy with this draft, but the condition is much simpler that way. Maybe it will be a step towords some better solution. |
|
Hey @hankem, I will paste the part of annonynous class here, that has a simpler condition, but is based on name resolving as described before. Give me heads up what do you think about it. |
2eac3cb to
22a38aa
Compare
hankem
left a comment
There was a problem hiding this comment.
I'm really sorry that it took me so long to get back to this! 🙄
But now that a new release is coming up, I'd really like to include your contribution...
I have one suggestion to further improve the rule. If you like it, we could squash all changes into one commit.
|
Hi @hankem 👋 Looks good, thanks. I applied your suggestion :) |
…not all with tests Signed-off-by: krzysztof-owczarek <krzysztofowczarek@icloud.com>
07d72da to
2e7af6a
Compare
|
Thank you! I've squashed all your commits and rebased them to the |
This will improve the existing rule to test that test classes reside in the same package as their implementation. In particular the rule:
Should not fail if there are multiple test classes with the same simple name and some of the classes have no tests at all
Issue: #1367