jenkinsci / peass-ci-plugin

Jenkins plugin for peass to support performance measurement in CI
https://plugins.jenkins.io/peass-ci/
GNU Affero General Public License v3.0
4 stars 13 forks source link

If jvmArgs already exist in build.gradle, extend them instead of adding a second jvmArgs-line #197

Closed mawHBT closed 1 year ago

mawHBT commented 1 year ago

What feature do you want to see added?

If you set --onlyOneCallRecording, peass configures aspectJ in jvmArgs in build.gradle: jvmArgs=["...-aspectj.jar"] This adds a second jvmArgs-line if one already exists, causing weird behaviour. In my case -Xmx from the first line took effect while -XX:+HeapDumpOnOutOfMemoryError did not. To avoid this, existing jvmArgs should just be extended.

Upstream changes

No response

DaGeRe commented 1 year ago

This should work with https://github.com/DaGeRe/peass/commit/5b789d7294cff37ee61c8f2bb7e8058054428c9d - could you test this? And if this is not working, could you specify a test case with your task definition syntax and eventually also a fix?

mawHBT commented 1 year ago

If a project has: jvmArgs=["-XX:+HeapDumpOnOutOfMemoryError", "-Xmx256m"] both arguments are added together as one argument, resulting in: jvmArgs=['anOldArgument', '-XX:+HeapDumpOnOutOfMemoryError, -Xmx256m'] This causes an error.

Also the jvmArgs for integrationTest are missing now. Last time I tried this, line jvmArgs=["-javaagent:${System.properties['user.home']}/.m2/repository/net/kieker-monitoring/kieker/1.15.1/kieker-1.15.1-aspectj.jar"] was added to integrationTest. This didn't happen this time.

mawHBT commented 1 year ago

I checked this locally by instrumentating a project with an existing argLine. This seems to work for test but I still miss: jvmArgs=["-javaagent:${System.properties['user.home']}/.m2/repository/net/kieker-monitoring/kieker/1.15.1/kieker-1.15.1-aspectj.jar" for integrationTest. This should be there, right? I started by cleaning up TestJVMArgsGradle for further testing, see PR#98.

DaGeRe commented 1 year ago

There might be a problem with integrationTest, it should also be instrumented. Could you create a unit test that proves that integrationTest is not adapted correctly, and eventually even provide a fix?

mawHBT commented 1 year ago

All I can say, is that the jvmArgs-line configuring aspectj is not added, if there is no (integration)test-block in the gradle file at all. This behaves the same for test and integrationTest.

I added testAspectJAddedWithOnlyOneCallRecordingAndTesttasksConfigured (green) and testAspectJAddedWithOnlyOneCallRecordingAndTesttaskNotConfigured (red) in this PR.

If I now comment the line Assert.assertTrue(integrationTestTask.contains(... in the green testAspectJAddedWithOnlyOneCallRecordingAndTesttasksConfigured and remove everything related to integrationTest from buildJVMArgsInMegabyte.gradle, the test suddenly fails. I don't understand why this happens and need further examination.

mawHBT commented 1 year ago

Since extension of jvmArgs works in general, I close this ticket. For the problem of not adding aspectJ-jar I opened #202.