RFR: 7307: Move org.openjdk.jmc.flightrecorder.configuration bundle from application to core [v5]
Christoph Langer
clanger at openjdk.org
Tue Oct 25 16:23:06 UTC 2022
On Fri, 21 Oct 2022 18:10:06 GMT, Alex Macdonald <aptmac at openjdk.org> wrote:
>> This PR addresses JMC-7307 [[0]](https://bugs.openjdk.java.net/browse/JMC-7307), in which it would be helpful to have `flightrecorder.configuration` distributed in jmc core.
>>
>> `flightrecorder.configuration` itself is a pretty straightforward movement of code, as it doesn't have any dependencies. Having said that, there are some flightrecorder-configuration-related classes in `controlpanel.ui` which would be nice to have in core as well, and would probably do well in `flightrecorder.configuration`. This includes VolatileStorageDelegate, and the 12 classes in `configuration.model.xml`. The `controlpanel.ui.configuration` unit tests can also come over to core, because they only tested the model xml code anyways.
>>
>> So in total, we're looking at `flightrecorder.configuration` and the former `controlpanel.ui.configuration` test coming over to core, and the following classes:
>> - org.openjdk.jmc.flightrecorder.configuration.model.VolatileStorageDelegate
>> - org.openjdk.jmc.flightrecorder.configuration.model.xml.IXMLValidator
>> - org.openjdk.jmc.flightrecorder.configuration.model.xml.JFCGrammar
>> - org.openjdk.jmc.flightrecorder.configuration.model.xml.JFCXMLValidator
>> - org.openjdk.jmc.flightrecorder.configuration.model.xml.PrettyPrinter
>> - org.openjdk.jmc.flightrecorder.configuration.model.xml.XMLAttribute
>> - org.openjdk.jmc.flightrecorder.configuration.model.xml.XMLAttributeInstance
>> - org.openjdk.jmc.flightrecorder.configuration.model.xml.XMLModel
>> - org.openjdk.jmc.flightrecorder.configuration.model.xml.XMLNode
>> - org.openjdk.jmc.flightrecorder.configuration.model.xml.XMLNodeType
>> - org.openjdk.jmc.flightrecorder.configuration.model.xml.XMLTag
>> - org.openjdk.jmc.flightrecorder.configuration.model.xml.XMLTagInstance
>> - org.openjdk.jmc.flightrecorder.configuration.model.xml.XMLValidationResult
>>
>> [0] https://bugs.openjdk.java.net/browse/JMC-7307
>
> Alex Macdonald has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 12 commits:
>
> - update license headers to 2022
> - fix package name in JFCXMLValidator logger
> - re-order flightrecorder.configuration.test in test pom
> - update license headers
> - migrate VolatileStorageDelegate to flightrecorder.configuration.model
> - revert .classpath files to their original from application
> - minor cleanup
> - move configuration.model.xml test from application to core; remove controlpanel.ui.configuration.test
>
> The controlpanel.ui.configuration only had the XML test, which is now
> located in flightrecorder.configuration.test in core. Currently all
> tests in both core and application will pass.
> - move flightrecorder.configuration coverage from application to core
> - temporarily silence test modules that are moved, will cleanup after
> - ... and 2 more: https://git.openjdk.org/jmc/compare/0e8e2004...9fad2d39
Hi @aptmac,
First of all, thanks for doing this refactoring. I now took the time to review this. I have added a few review comments inline.
Furthermore, in project flightrecorder.configuration.test, please move the test classes from src/test/java to src/main/java, as in the other test projects. That is the current convention. To make it work, you have to add this to pom.xml:
` <build>
<testSourceDirectory>${project.basedir}/src/main/java</testSourceDirectory>
</build>`
Then, in core/org.openjdk.jmc.flightrecorder.configuration/META-INF/MANIFEST.MF, it could be better/required now to use
`Require-Bundle: org.openjdk.jmc.common,
+ org.openjdk.jmc.flightrecorder`
instead of
`Import-Package: org.openjdk.jmc.flightrecorder.internal`
Other than that, it looks good to me.
See above.
core/tests/org.openjdk.jmc.common.test/src/main/java/org/openjdk/jmc/common/util/MethodToolkitTest.java line 41:
> 39:
> 40: import org.junit.Test;
> 41: import org.openjdk.jmc.common.util.MethodToolkit;
This import is not needed atm since the test class lives in package org.openjdk.jmc.common.util from module org.openjdk.jmc:common. A better way for these tests would be to change their package to org.openjdk.jmc.common.util.test but that should be addressed in another PR.
core/tests/org.openjdk.jmc.common.test/src/main/java/org/openjdk/jmc/common/util/SortedHeadTest.java line 42:
> 40:
> 41: import org.junit.Test;
> 42: import org.openjdk.jmc.common.util.SortedHead;
Same as my comment in MethodToolkitTest.java
core/tests/org.openjdk.jmc.common.test/src/main/java/org/openjdk/jmc/common/util/TypeHandlingTest.java line 38:
> 36:
> 37: import org.junit.Test;
> 38: import org.openjdk.jmc.common.util.TypeHandling;
Same as my comment in MethodToolkitTest.java
core/tests/org.openjdk.jmc.common.test/src/main/java/org/openjdk/jmc/common/version/JavaVMVersionToolkitTest.java line 33:
> 31: * WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> 32: */
> 33: package org.openjdk.jmc.common.test.version;
This test should then be moved into subdirectory org/openjdk/jmc/common/test/version. But I think also in a separate change.
core/tests/org.openjdk.jmc.common.test/src/main/java/org/openjdk/jmc/common/version/JavaVersionTest.java line 33:
> 31: * WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> 32: */
> 33: package org.openjdk.jmc.common.test.version;
This test should then be moved into subdirectory org/openjdk/jmc/common/test/version. But I think also in a separate change.
core/tests/org.openjdk.jmc.flightrecorder.configuration.test/build.properties line 2:
> 1: #
> 2: # Copyright (c) 2018, 2021, Oracle and/or its affiliates. All rights reserved.
Wrong Copyright year. If the file content doesn't change, the copyright doesn't need to be updated, anyway.
core/tests/org.openjdk.jmc.flightrecorder.configuration.test/pom.xml line 3:
> 1: <?xml version="1.0" encoding="UTF-8"?>
> 2: <!--
> 3: Copyright (c) 2018, 2021, Oracle and/or its affiliates. All rights reserved.
Needs 2022, I guess.
core/tests/org.openjdk.jmc.flightrecorder.configuration.test/pom.xml line 62:
> 60: <groupId>junit</groupId>
> 61: <artifactId>junit</artifactId>
> 62: <version>${junit.version}</version>
Don't add the version here, it's inherited from parent.
core/tests/org.openjdk.jmc.flightrecorder.configuration.test/src/test/java/org/openjdk/jmc/flightrecorder/configuration/OfflineEventOptionsTest.java line 2:
> 1: /*
> 2: * Copyright (c) 2018, 2021, Oracle and/or its affiliates. All rights reserved.
Update to 2022.
core/tests/org.openjdk.jmc.flightrecorder.configuration.test/src/test/java/org/openjdk/jmc/flightrecorder/configuration/OfflineEventOptionsTest.java line 33:
> (failed to retrieve contents of file, check the PR for context)
why change the package?
core/tests/org.openjdk.jmc.flightrecorder.configuration.test/src/test/java/org/openjdk/jmc/flightrecorder/configuration/model/xml/XMLTagInstanceTest.java line 2:
> 1: /*
> 2: * Copyright (c) 2018, 2021, Oracle and/or its affiliates. All rights reserved.
2022
core/tests/org.openjdk.jmc.flightrecorder.configuration.test/src/test/java/org/openjdk/jmc/flightrecorder/configuration/model/xml/XMLTagInstanceTest.java line 33:
> (failed to retrieve contents of file, check the PR for context)
Better move it to package org.openjdk.jmc.flightrecorder.configuration.model.xml.test.
-------------
PR: https://git.openjdk.org/jmc/pull/299Changes requested by clanger (Committer).
More information about the jmc-dev
mailing list