Runing pitclipse with Eclipse 2023-12 and Java 21 fails

NolwennD commented 9 months ago

Bug description

Java 21 is not supported by the packaged pitest (1.6.8)

Exception in thread "main" java.lang.IllegalArgumentException: Unsupported class file major version 65

Expected behavior

Run mutation testing

How to reproduce

Have a JDK 21 on your computer.

git clone git@github.com:NolwennD/UglyMolKKy-RefactoringKata.git

import existing maven project from java folder

run pitest on MolkkyTest

Additional context

Everything is fine with pitest and pitest-maven in version 1.15.3 with this command:

mvn test-compile org.pitest:pitest-maven:mutationCoverage

Pitclipse version: Installed from marketplace (v2.2.1.v20230330-0839)
Eclipse IDE version:
2023-12

LorenzoBettini commented 9 months ago

@NolwennD thanks for reporting this! I guess it's time to update the version of pit we're using in pitclipse.

LorenzoBettini commented 5 months ago

@echebbi do you have some time to have a look at this or provide some hints?

echebbi commented 5 months ago

@LorenzoBettini sure, I'll take a look!

LorenzoBettini commented 5 months ago

@echebbi thanks! I tried to port it to the new version of pit, but there was an API breakage and I wasn't able to adapt the runtime code on which I don't have internal knowledge.

echebbi commented 5 months ago

Implementation in progress on branch 221-support-java-21.

@LorenzoBettini as of PIT 1.7.5 some "research oriented" mutators are not provided by PIT anymore and have been moved to an external PIT plugin (see https://github.com/hcoles/pitest/pull/993). This is outlined by this test (the lines that are commented out are v1.6.8's mutators, the new lines are the new mutators). How do you think we should address this change? Should we integrate the new plugin to keep all our current mutators?

PIT's readme states that:

[...] these mutators are not recommended for general use

hence I would suggest to forget them for now. We can add them again in the future, and maybe make it explicit in the UI that they are "research oriented" mutators.

LorenzoBettini commented 5 months ago

@echebbi , first of all, thanks for the hard work on porting :)

I think we should follow what PIT does, and forget about them for the moment. (especially if these mutators are not recommended). I'd like to keep Pitclipse as simple as possible in that respect.

Please, let me know if I can help somehow.

I saw that you moved the Java version to 21 in GitHub Actions. Maybe that's not required: I seem to understand that the new version of PIT supports Java 21, but not requires that (hopefully). I bet Java 21 will give problems with the old version of Tycho we're still using. In that respect, in a separate PR, I can update our build to use the new version of Tycho (4.x), which requires Java 17 for the build. What do you think? I seem to remember, though, that some unit tests fail with Java 17 due to its new way of dealing with streams and files, but that could be adjusted.

LorenzoBettini commented 5 months ago

@echebbi concerning the failure in the CI (I see you have already moved to Tycho 4), you need to use this step to setup a more recent version of Maven required by Tycho 4:

    - name: Set up Maven
      uses: stCarolas/setup-maven@v5
      with:
        maven-version: 3.9.6

echebbi commented 5 months ago

@LorenzoBettini Thanks, I'm not familiar with GitHub Actions so that's helpful!

I saw that you moved the Java version to 21 in GitHub Actions. Maybe that's not required

I agree, it was just a quick workaround because the CI was failing due to IProgressMonitor being compiled with Java 17. That's surprising because the environment should be set by the target platform. Please let me know if you have any pointer on that issue.

LorenzoBettini commented 5 months ago

I agree, it was just a quick workaround because the CI was failing due to IProgressMonitor being compiled with Java 17. That's surprising because the environment should be set by the target platform. Please let me know if you have any pointer on that issue.

From the log I seem to understand it's actually Tycho itself that somehow takes a version of progress monitor from a recent version of an Eclipse maven artifact: it still hasn't started to resolve the target platform, it happens in the early stage. In any case, we should upgrade the build to Java 17 anyway now :)

echebbi commented 5 months ago

@LorenzoBettini fine by me, feel free to update Java and Tycho in a dedicated PR and I'll update my branch once the PR is merged

LorenzoBettini commented 5 months ago

@echebbi OK, but did you find a way to fix the unit tests related to files in Java 17?

echebbi commented 5 months ago

@LorenzoBettini I'm not sure, are you talking about the failing tests in ObjectStreamSocketTest? I figured out that we can remove the call to spy, which fixes the tests:

The input stream doesn't seem to be used as a spy anyway so I believe that spy is a leftover from older tests and can be safely removed.

LorenzoBettini commented 5 months ago

@echebbi I remembered a few failing tests, a few months ago, but I don't remember. I'll check with your modification.

pitest / pitclipse