review: refactor: Update EnumsTest to JUnit 5

[SirYwell]

As I and @I-Al-Istannen were looking into issues with implicit modifiers recently, I decided to update this test class as it contains several tests which I consider problematic and might need changes in near future.

I tried to infer the contract for the tests which didn't have one before. I hope I got it right more or less.

Now to the problems I encountered:

testNestedPrivateEnumValues

The enums are currently not marked as final. In my opinion, they should have an implicit final modifier, as the JLS $ 8.9 says:

An enum class is implicitly final unless its declaration contains at least one enum constant that has a class body (§8.9.1).

The second part of that sentence brings us to a second issue that is not covered for now. It is also related to #4129 (see how the implicit check adds () even if they don't exist in the original code because of the constant being an anonymous class). I also added TODOs for the other tests that expect specific printer output.

Another issue is the visibility of the enum constants. The test currently expects them to have the same visibility modifiers as their class. This was introduced in #1999 but I wasn't able to reproduce that but I also didn't find any more specific information in the JLS. It might be a version specific implementation detail, but for examle - with Java 16 - looking at the output of `javap -v NestedEnums$ProtectedENUM.class` (after compiling the `NestedEnum` class with `javac` shows the following for the VALUE constant:

  public static final NestedEnums$ProtectedENUM VALUE;
    descriptor: LNestedEnums$ProtectedENUM;
    flags: (0x4019) ACC_PUBLIC, ACC_STATIC, ACC_FINAL, ACC_ENUM

(while the test expects it to be protected)

---

In foresight

Java 17 will introduce sealed classes. While changes are still possible there, there is a very interesting point that makes more sense as more as I think about it: Enums are still final if no constant has a body, but become sealed once a constant has a body. The anonymous class represented with that body is additionally final. The EnumsTest class likely needs to be updated once that feature is supported, with version-specific tests for that.

---

Edit

The testGetAllMethods test revealed that the SignatureBasedSortedSet basically violates the contract of Set. The contains method returns true while iterating and doing equals checks returns false eventually:

spoon/src/test/java/spoon/test/enums/EnumsTest.java

Lines 111 to 113 in 6f25216

	assertTrue(burritos.getAllMethods().contains(name));
	// this does not work due to the SignatureBasedSortedSet violating the Set contract
	// assertThat(burritos.getAllMethods(), hasItem(name));

I'm not sure what the exact reasoning is, maybe there are other approaches that could be considered. Should I open a separate issue for that?

[SirYwell]

@monperrus @slarse do you know more about the visibility situation? In my opinion that's currently a bug, but as it was explicitly added in #1999 I'm not sure if I'm missing something here.

[monperrus]

do you know more about the visibility situation?

No. If there is no info in the JLS, the compiler is the spec (try different combinations modifiers/access and see if it compiles).

[SirYwell]

No. If there is no info in the JLS, the compiler is the spec (try different combinations modifiers/access and see if it compiles).

The problem is that the modifiers are implicit. Explicit modifiers are not allowed at all, but the produced bytecode marks the fields with ACC_PUBLIC (see my example above), while they inherit the visibility from their declaring class in spoon currently. It might have been like that in older javac versions, but then I'd say it's still wrong for current versions.

[monperrus]

Thanks for the analysis. Let's fix the bug then.

[slarse]

Enum constants are syntactical sugar, they don't semantically have visibility themselves. The fields derived from them do, however. JLS says the following about those fields (from section 8.9.2 of JLS SE17):

For each enum constant c declared in the body of the declaration of E, E has an implicitly declared public static final field of type E that has the same name as c. The field has a variable initializer which instantiates E and passes any arguments of c to the constructor chosen for E. The field has the same annotations as c (if any).

So, public static final is as correct as it gets. It may be considered a little bit semantically and syntactically quirky to even attribute visibility to enum constants, but I suppose in Spoon they are really represented as the fields derived from them. Which is probably how most people view enum constants.

[SirYwell]

As CtEnumValue extends CtField, I guess it makes sense to have the modifiers of the field there too.

This refactoring somehow ended up in bug fixing, I hope that's okay.
In 3136d56 I also fixed a bug where enum values created by the JavaReflectionTreeBuilder are marked as abstract (the flag is used for both abstract and enum internally). Should I add a test case for that too?

The SignatureBasedSortedSet still makes me thinking, so I'm happy about any input for that too.

[slarse]

Woops, this has been left unattended for a while now.

This refactoring somehow ended up in bug fixing, I hope that's okay.

This is a little bit of a problem. Making non-trivial changes to test and production code in a single PR, beyond what is strictly necessary, is something of a no-no. Would it be much effort for you to split this into one PR that actually just updates the test to JUnit5, and one PR that fixes the modifier bug? That would be much preferable from a reviewer perspective.

SignatureBasedSortedSet basically violates the contract of Set.

I agree that it violates the set contract. What to do about that should be discussed in a separate issue, and is (as far as I can understand) orthogonal to updating this class to use JUnit5.

For the sake of explicitness: It violates the set contract by imposing an ordering (with the custom comparator) that is not consistent with equals (i.e. it does not satisfy "c.compare(e1, e2)==0 has the same boolean value as e1.equals(e2) for every e1 and e2 in S)

[SirYwell]

Would it be much effort for you to split this into one PR that actually just updates the test to JUnit5, and one PR that fixes the modifier bug?

No, that should be doable, When updating the tests first, they will remain wrong (as they test something that is wrong), which feels a bit weird.

Same basically applies for the wrong abstract modifier coming from the JavaReflectionTreeBuilder (as I had a failing test here https://github.com/INRIA/spoon/runs/3600960090#step:10:565 before fixing that for some reason).

So it's kind of a chicken and egg problem...

---

Edit: I thought I could extract the issue with the JavaReflectionTreeBuilder, but those are basically two different issues too:

• spoon/src/main/java/spoon/support/visitor/java/JavaReflectionTreeBuilder.java

  Line 313 in 2fff4c5

  setModifier(ctEnumValue, field.getDeclaringClass().getModifiers(), field.getDeclaringClass().getDeclaringClass());

  previously, the modifiers were extracted from the wrong targets (once the enum class instead of the field, once from Class itself)
• abstract was applied to enum values, likely also leading to this confusing comment:

  spoon/src/main/java/spoon/support/visitor/java/JavaReflectionTreeBuilder.java

  Lines 500 to 502 in 2fff4c5

  	if (ctModifiable instanceof CtEnum) {
  	//enum must not be declared abstract (even if it can be made abstract see CtStatementImpl.InsertType)
  	//as stated in java lang spec https://docs.oracle.com/javase/specs/jls/se7/html/jls-8.html#jls-8.9

Both make the same test fail because the test compares the reflection model with the jdt model which is wrong too...

[slarse]

No, that should be doable, When updating the tests first, they will remain wrong (as they test something that is wrong), which feels a bit weird.

Yes, but it's not the concern of a PR meant to update the test framework to also fix bugs. It's very common to find semantic problems when refactoring code (this is a refactoring of the test suite), and the "proper" way to proceed then is to document those bugs and continue with the refactoring.

Take the reviewer's perspective. If a PR migrates a test class from JUnit4 to JUnit5, the only thing the reviewer needs to do is to verify that the semantics of the tests are unchanged. But if the PR also changes the semantics of the test case (on purpose), then the reviewer needs to verify that the semantics of the tests are unchanged except for where they purposefully have been changed, and also check that the production code is updated correctly. It becomes very easy to make a mistake as a reviewer.

To summarize, when refactoring and you find bugs: document the bugs, complete the refactoring, then fix the bugs separately :)

[SirYwell]

To summarize, when refactoring and you find bugs: document the bugs, complete the refactoring, then fix the bugs separately :)

That makes sense, I'll do so.

[SirYwell]

I removed the fixes and commented the affected test code accordingly. Should I open issues for the bugs or should I just open PRs for them after this is merged?

[slarse]

Thanks @SirYwell, I'll have a look at this tonight

Should I open issues for the bugs or should I just open PRs for them after this is merged?

There's nothing in the contributing guidelines saying an issue must exist for a PR, so I think you can do it whichever way you feel like. If it's a straightforward bugfix that doesn't require any particular discussion about how to fix it, I think it's fine to just open a PR without having an issue.

For complex stuff I personally prefer to always open an issue first.

[slarse]

Okay, had a look now.

I would not say that this PR is an update of EnumsTest to JUnit5, such a PR would be small. This is rather a very significant overhaul of the entire test class. I think it's a bit much, especially as most of the test changes aren't related to each other.

For example, when just considering updating to JUnit5, changing an assertEquals to an assertThat is not in any way necessary. Especially seeing as Hamcrest matchers are available for JUnit4 as well, it's not even related.

Let's continue the PR with it's current scope, most changes are for the better (see my comments on the things I don't think are improvements). But for future reference, this is a bit too many too unrelated things at the same time.

[slarse]

This is a production code change, it should not be necessary to migrate the tests to JUnit5. Where exactly is this utilized?

[SirYwell]

Failing tests where modifiers don't match were hard (impossible) to read. I can move this change to the bug fixing PR too, there it would be more related probably.

[slarse]

Right, I see. That's a fine enough justification for that.

[slarse]

I don't think this is an improvement over the original test. While the original has the problem that it's 4 tests in one, it's very easy to read. The refactored version has 2 mutually exclusive branching paths (in general, avoid branches in tests) and also requires one to understand the non-trivial parameterization.

I favor the original test, it's much easier to grasp. I'm guessing you have a plan here for when you fix the modifier bug, but that's not a concern of this PR. Could you revert to the original? Feel free to reintroduce the parameterized test if it actually simplifies things in a future PR.

[slarse]

This comment is noise for this test, and it's very unlikely to be removed if SignatureBasedSortedSet is fixed. I'd remove it.

[slarse]

Thanks @SirYwell, this looks good to merge for me.

It's great that you're giving the test suite some much needed attention, such things tend to be neglected in favor of working on production code :)