search

mapstruct / mapstruct-idea

An IntelliJ IDEA plugin for working with MapStruct
Other
141 stars 38 forks source link

Improve build/test infrastructure to support IDEA 2022.2/2022.3 and Java 17 #113

Closed unshare closed 1 year ago

unshare commented 2 years ago
unshare commented 2 years ago

@filiphr, please take a look. We've got serious breakages ahead with the 2022.3 release: 25/164 test cases fail. Sorry to upgrade Gradle build -- I had to do it actually have the tests run. Gradle 6.9.1 won't run on Java 17 the way intellij-gradle-plugin works. 223.6646.99-EAP-SNAPSHOT is added because it's less b0rken than LATEST on my machine.

filiphr commented 1 year ago

Thanks a lot for your work on this @unshare. I'll have a look at the failing tests on 2022.3. I don't think that the failures are that big, it is only some small text formatting changes that have been done in IntelliJ

filiphr commented 1 year ago

@unshare I checked this out locally and I've fixed some of the problems in the tests.

By the way, what kind of a machine do you have? I have an Intel Mac and I couldn't run the tests. I had to use a snapshot version of the Gradle IntelliJ Plugin to run the tests properly. The build is also green for me when using LATEST-EAP-SNAPSHOT. I think that we can remove 223.6646.99-EAP-SNAPSHOT from the GItHub matrix build

unshare commented 1 year ago

Great! You did manage to get a green checkmark. As for my machine and my test issues, I predominantly run Windows desktops both at work and at home. And I tend to agree the issues I experience are Windows-specific.

  1. There's a lot of lamenting on JNA not available for a new feature: using OS-native certificates. It litters logs yet is completely harmless for our use case.
  2. There's a development around file system implementation that is definitely Windows-specific: https://github.com/JetBrains/intellij-community/commit/9793bb4cf40b0ed14bcff9e9fd95feb481781bf2 It triggers errors like these: java.lang.IllegalAccessError: class com.intellij.openapi.vfs.impl.local.LocalFileSystemImpl (in unnamed module @0x5f4da5c3) cannot access class sun.nio.fs.BasicFileAttributesHolder (in module java.base) because module java.base does not export sun.nio.fs to unnamed module @0x5f4da5c3 223.6646.99 just happens to be the last available build without it. This issue does affect us as we do file access directly or indirectly.
    Stack trace for 223 EAP follows, but beware `master` is already pretty much refactored around this
    java.lang.IllegalAccessError: class com.intellij.openapi.vfs.impl.local.LocalFileSystemImpl (in unnamed module @0x5f4da5c3) cannot access class sun.nio.fs.BasicFileAttributesHolder (in module java.base) because module java.base does not export sun.nio.fs to unnamed module @0x5f4da5c3
        at com.intellij.openapi.vfs.impl.local.LocalFileSystemImpl.listWithAttributes(LocalFileSystemImpl.java:285)
        at com.intellij.openapi.vfs.impl.local.LocalFileSystemImpl.lambda$new$0(LocalFileSystemImpl.java:42)
        at com.intellij.openapi.vfs.DiskQueryRelay.accessDiskWithCheckCanceled(DiskQueryRelay.java:47)
        at com.intellij.openapi.vfs.impl.local.LocalFileSystemImpl.listWithCaching(LocalFileSystemImpl.java:249)
        at com.intellij.openapi.vfs.newvfs.persistent.PersistentFSImpl.persistAllChildren(PersistentFSImpl.java:201)
        at com.intellij.openapi.vfs.newvfs.persistent.PersistentFSImpl.listAll(PersistentFSImpl.java:267)
        at com.intellij.openapi.vfs.newvfs.impl.VirtualDirectoryImpl.loadAllChildren(VirtualDirectoryImpl.java:396)
        at com.intellij.openapi.vfs.newvfs.impl.VirtualDirectoryImpl.getChildren(VirtualDirectoryImpl.java:389)
        at com.intellij.testFramework.UsefulTestCase$2.visitFile(UsefulTestCase.java:1136)
        at com.intellij.openapi.vfs.VirtualFileVisitor.visitFileEx(VirtualFileVisitor.java:106)
        at com.intellij.openapi.vfs.VfsUtilCore.visitChildrenRecursively(VfsUtilCore.java:295)
        at com.intellij.testFramework.UsefulTestCase.refreshRecursively(UsefulTestCase.java:1133)
        at com.intellij.testFramework.fixtures.impl.LightTempDirTestFixtureImpl$1.compute(LightTempDirTestFixtureImpl.java:85)
        at com.intellij.testFramework.fixtures.impl.LightTempDirTestFixtureImpl$1.compute(LightTempDirTestFixtureImpl.java:79)
        at com.intellij.openapi.application.impl.ApplicationImpl.lambda$runWriteAction$10(ApplicationImpl.java:964)
        at com.intellij.openapi.application.impl.ApplicationImpl.runWriteActionWithClass(ApplicationImpl.java:943)
        at com.intellij.openapi.application.impl.ApplicationImpl.runWriteAction(ApplicationImpl.java:964)
        at com.intellij.testFramework.fixtures.impl.LightTempDirTestFixtureImpl.copyAll(LightTempDirTestFixtureImpl.java:79)
        at com.intellij.testFramework.fixtures.impl.LightTempDirTestFixtureImpl.copyAll(LightTempDirTestFixtureImpl.java:74)
        at com.intellij.testFramework.fixtures.impl.CodeInsightTestFixtureImpl.copyDirectoryToProject(CodeInsightTestFixtureImpl.java:476)
        at org.mapstruct.intellij.MapstructBaseCompletionTestCase.addDirectoryToProject(MapstructBaseCompletionTestCase.java:52)
        at org.mapstruct.intellij.expression.JavaExpressionInjectionTest.setUp(JavaExpressionInjectionTest.java:172)

I've always suspected IntelliJ does some workarounds against Java modularity (Project Jigsaw), and this one is kind of proof. The JetBrains code is not broken, just called in a way not originally intended.

Removed the said last known good build from CI as it makes no difference when tested on a Linux-based CI.

unshare commented 1 year ago

@filiphr , please also take a look at #112 as it's this minor edit that triggered the whole investigation. I do rely on IDEA's code inspection for multiple reasons. Hence my tendency to squash false positives.

filiphr commented 1 year ago

I also had a lot of JNA not available stuff locally. Using 1.10.0-SNAPSHOT from the gradle-intellij-plugin took all of that away. Seems like the plugin is configuring the build somehow and disables some things for IntelliJ.

Thanks a lot for your work on this. I'll merge it shortly and yes #112 was next on my list to review and merge. Didn't have the time yesterday (I wanted to check it out locally and see what was causing this)