Could you review the changes? I think it should be ok now, thanks.

Thank you @polarene and sorry for the slow feedback, I will have a look during this week.

Hi @scordio no problems at all, take your time! Would you mind to tag the PR with hacktoberfest-accepted so it counts for this Hacktoberfest? Thank you

assertj-core has now the hacktoberfest topic so this PR should be considered valid once it is merged.

Thank you very much!

Integrated, thanks @polarene!

Thanks for merging @scordio! Only a small disappointment is that I would have liked to be notified about those last changes in the tests because I had a rationale behind, but it's fine anyway. Thanks for your support, looking forward to contributing again to Assertj!

I noticed that one test was missing and I spotted some changes I'd have done around. As I promised to merge the PR by the end of the week I did them by myself, but nothing is written in stone: what would you do differently?

@polarene we wanted to release 3.18.0 this last week end, even if the PR was merged, we are keen to hear what you have to say.
And thanks for your contribution btw!

Hi guys, sorry if my words didn't come out right. I really appreciate your support and hard work, in fact, congratulations on letting this PR in time for 3.18.0, great job!

All the latest changes from Stefano are ok indeed, I just wanted to point out that the modifications in the Classes_assertHasPackage_* tests:

• erased a specific test case I wrote, which checked the mismatch between packages that have a common prefix (like "com.foo" and "com.foo.bar")
• eliminated a parametric test where the rationale was to check against a Package instance obtained both from Class#getPackage() and Package#getPackage() -- since the latter is deprecated in Java 9+, when AssertJ migrates from 8 it should be checked against ClassLoader.getDefinedPackage(java.lang.String)
• replaced the Class instances used in the test cases with Object and Collection, but I think those I chose initially made the tests more entertaining (Jedi), following the spirit of this library, like the examples with LoTR characters, and more specific to the point (MyClass) as its package and package prefix were more evident for the cases
• eliminated some // GIVEN blocks which I think made the tests more readable

That's it, thanks for reading so far.

Hi @polarene,

• erased a specific test case I wrote, which checked the mismatch between packages that have a common prefix (like "com.foo" and "com.foo.bar")

Here I assumed that classes have only one package which is uniquely identified, that's why I generalized it to should_fail_if_packageName_does_not_match. Do you see it differently?

• eliminated a parametric test where the rationale was to check against a Package instance obtained both from Class#getPackage() and Package#getPackage() -- since the latter is deprecated in Java 9+, when AssertJ migrates from 8 it should be checked against ClassLoader.getDefinedPackage(java.lang.String)

Here I didn't want to test how the Package instance retrieval is done (which is JDK business) unless we have a reason for it. To the best of my knowledge, we should expect the same result between Class#getPackage(), Package#getPackage(String) and the Java 9+ ClassLoader.getDefinedPackage(String).

Also, I just realized we could even write:

  @ParameterizedTest
  @ValueSource(strings = "java.util")
  void should_fail_if_package_does_not_match(Package aPackage) {
    ...
  }

and JUnit 5 will resolve it with its implicit argument converter. The latter is actually my preference and I would update the test with it.

What do you think?

EDIT: I took a look at the JUnit 5 internals, java.lang.Package is not explicitly resolved but it works because of the factory method search in the Fallback String-to-Object Conversion, which is actually picking up the deprecated Package#getPackage(). It would be better to pick up ClassLoader.getDefinedPackage(java.lang.String) on Java 9+. I'll open an issue to discuss it with JUnit5.

• replaced the Class instances used in the test cases with Object and Collection, but I think those I chose initially made the tests more entertaining (Jedi), following the spirit of this library, like the examples with LoTR characters, and more specific to the point (MyClass) as its package and package prefix were more evident for the cases

I preferred to use well-known JDK types to avoid jumping back and forth in the classes and understand where the Jedi class is located. In general, I like the entertaining test style, but I think it's better suited for the javadoc and for assertj-examples. If there is an option to avoid the usage of custom code in the tests, I vote for it.

• eliminated some // GIVEN blocks which I think made the tests more readable

In general, I tend to shorten tests where local variables are single values and can be inline, but I see your point. I'll put them back.

Hi @scordio, thank you very much for answering my points, I see your reasons and generally agree on everything.

Here I assumed that classes have only one package which is uniquely identified, that's why I generalized it to should_fail_if_packageName_does_not_match. Do you see it differently?

Basically you're right, packages are unique for a given class, but what I wanted to show with those tests was to make clear that hasPackage() would fail even for a "sub-package", that is one that has an existing package as a prefix. Although Java considers a sub-package to be different from its ancestor, I wanted to make it clear both for users and contributors of AssertJ that the assertion is following the language specification, in fact, this is reflected in one of the Javadoc examples.
Tell me if it makes sense for you to include such a test or if the Javadoc suffices.

Here I didn't want to test how the Package instance retrieval is done (which is JDK business) unless we have a reason for it. To the best of my knowledge, we should expect the same result between Class#getPackage(), Package#getPackage(String) and the Java 9+ ClassLoader.getDefinedPackage(String).

Ok maybe I confused test responsibilities, but I really wanted to assert that the instances retrieved in different ways were checked consistently, to cover corner cases, and to be future-proof whenever something changes in the next JDK versions, so those tests could signal it. I wrote it exactly to assert that we expect the same result from the various retrieval methods.

JUnit 5 conversions
Nice, let's discuss that in another issue, it seems a feature worth investigating. Only thing is that I wanted to make the check explicit depending on how you retrieve the package, see above. But yes, let's discuss this in a separate thread.

I preferred to use well-known JDK types [...]

Got it, I understand your reason, thanks for clarifying. Now I know for future PRs: funny stuff only in Javadoc. The thing was, after looking around the code base I found some custom classes uses for tests here and there, so I thought it was a legit use, also I didn't write Jedi, found it in ClassesBaseTest.

Thanks for restoring the GIVEN blocks, I appreciate it!