Refactoring AssemblyMojo#299
Refactoring AssemblyMojo#299kemitix wants to merge 161 commits intoapache:masterfrom kemitix:assembly-mojo-refactoring-4.2.x
Conversation
maven-project is pinned at 2.0.9, separate from maven.version as the tooling module updates to maven 3.x which does not have a maven-project. Even though maven-project doesn't appear to be used and should probably be removed, not pinning the version in dependencyManagement will cause mvn to fail with an unresolved artifact.
Updated MavenUtilTest
Extracts the doExecute() method into it's own class, passing in AssemblyMojo as a parameter, addorned with suitable set/get methods.
…cies, profiles and features
…ileDirectory is required
Split into individual tests for each type of dependency.
|
I will review the changes. Can you fix the conflicts? |
|
On it. |
* master: (70 commits) [KARAF-5170] Use try-with-resources [KARAF-5169] remove redundant type specifiers [KARAF-5168] Replace old-style loops [KARAF-5154] Upgrade to Felix Framework 5.6.4 [KARAF-5153] Upgrade to Felix BundleRepository 2.0.10 [KARAF-5152] Upgrade to commons-compress 1.14 [KARAF-5151] Upgrade to Aries Transaction Manager 1.3.3 This closes #305 KARAF-5162: use new Map methods [KARAF-5150] Upgrade to Aries Blueprint Core 1.8.1 Fix warnings [KARAF-5149] Upgrade to JNA 4.4.0 [KARAF-5123] Fix remove repo issue Add toString to RepositoryImpl [KARAF-5159] Upgrade to Felix WebConsole 4.3.4 [KARAF-5148] Remove use of org.json replaced by org.apache.felix.utils.json Fix some minor terminal issues Fix warnings [KARAF-5123] Add test for issue [KARAF-5147] Upgrade to pax-web-6.0.4 ...
|
Done. :) |
|
Thanks. I have looked into the code. I think the big benefit it provides is improved testing of AssemblyMojo. I do not like the refactoring of AssemblyMojo itself too much. It increases the number of classes a lot and even the number of lines in AssemblyMojo. I did some statistics with cloc: assembly-mojo-refactoring-4.2.x (sources without tests): I think AssemblyMojo needs some refactoring but we should do it less extensively. You also changed some other stuff like removing the DependencyHelper30 which I think is a good idea. So I would like to check if we can salvage parts of your work while not applying all. I got most of the tests working 2 errors and 4 failures remain. So if you are interested to work on this I propose the following:
Then from this base we can discuss how to improve AssemblyMojo. |
|
Okay.
I can do that. I won't be able to get back to this for a few days though.
…On Thu, 1 Jun 2017 at 14:37 Christian Schneider ***@***.***> wrote:
Thanks.
I have looked into the code. I think the big benefit it provides is
improved testing of AssemblyMojo.
I do not like the refactoring of AssemblyMojo itself too much. It
increases the number of classes a lot and even the number of lines in
AssemblyMojo.
I did some statistics with cloc:
master (sources without tests):
Java 27 804 1291 5153
AssemblyMojo.java
Java 1 63 129 388
assembly-mojo-refactoring-4.2.x (sources without tests):
Java 39 972 1497 5472
AssemblyMojo.java
Java 1 131 118 430
I think AssemblyMojo needs some refactoring but we should do it less
extensively.
You also changed some other stuff like removing the DependencyHelper30
which I think is a good idea.
I guess it was simply needed after the dependency upgrades but it is a
good change in itself.
So I would like to check if we can salvage parts of your work while not
applying all.
I experimented a bit with picking the AssemblyMojoSystemTest from your
code and everything it needs to work. I pushed what I have now to
https://github.com/cschneider/karaf/tree/assembly-mojo-tests/tooling/karaf-maven-plugin
I got most of the tests working 2 errors and 4 failures remain.
So if you are interested to work on this I propose the following:
1. In one PR provide an update to the poms and removal of the
DependencyHelper30 mechanism.
2. In a second PR provide a fully working implementation of the
AssemblyMojoSystemTest (like I started with) while doing only minimal
changes in production code.
Then from this base we can discuss how to improve AssemblyMojo.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#299 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/ABGDZbcE_lc1e0fO2S5RJxDLD5LSeZOKks5r_r6pgaJpZM4NYHsr>
.
|
|
@cschneider @kemitix if #314 is merged, can we close this one? I'm working on new issue I created (Clean up AssemblyMojo - related to Processor mechanism for feature definitions (a.k.a. "better overrides")), so there'll be a bit more refactoring soon |
|
FAILURE --none-- |
|
@grgrzybek Guess I never did get back to this. Closing it to clear the way. |
|
Thanks @kemitix for response. I even tried squashing your commits to single one and verify if it's still possible to merge. Your refactoring is impressive, but there were many conflicts, as AssemblyMojo changed since your PR. I started KARAF-5468 and focused on adding documentation, additional logging and making it consistent with Builder, profiles and features processing. I'm working on it while having your branch opened for inspiration ;) |
ENTESB-9638 Update AMQ 7.1.1 clients in Fuse to AMQ 7.2 so that their are aligned with AMQ 7.2 Broker
4 participants
AssemblyMojois a 580 line file with a single 257-line method accounting for the bulk of it. It also has no unit tests.This creates system tests for
AssemblyMojoand refactors it into a more manageable structure, all within a newassemblypackage, with suitable unit tests. Code coverage is 100% from either the system tests or the unit tests.