[MNG-6294] Convert MavenPluginValidator into a plexus component #134
Conversation
eolivelli
left a comment
There was a problem hiding this comment.
I am not an expert of Plexus, so my I am just giving my 2 cents.
The refactor looks good to me.
I left a comment about the unit tests, we can add some more cases in order to have better coverage and expecially in order to be sure that we are testing what we think we are testing
| descriptor.setArtifactId( "maven-it-plugin" ); | ||
| List<String> errors = new ArrayList<>(); | ||
| mavenPluginValidator.validate( plugin, descriptor, errors ); | ||
| assertFalse( errors.isEmpty() ); |
There was a problem hiding this comment.
nit: we should test more the "errors" result, it is not clear that we are catching the error we are generating with the lines above.
We have three validations which can fire an error, we should test every of them.
|
Willing to merge if someone can extend the tests. |
|
Hi, I currently don't have time to continue with this PR, but @mizdebsk will take it over and complete the tests. Please be patient. |
|
Ack, I will take over this PR from @msimacek - rebase it and extend unit tests. |
|
Thanks guys, no need to rush. Will just wait. |
|
hmm, we need to rethink this a bit, I'm not sure if passing a list to get the errors is the preferred way. Also I would consider to use a Problem or something similar instead, so we can provide more detailed information. |
Modified by: Guillaume Dufour <guillaume.duff@gmail.com> This closes apache#134 and closes apache#470
…#134) issue apache#131 apache#132 * fix resolution order in reverse issue in <flattenDependencyMode>all</flattenDependencyMode> * fix missing dependency in flattened pom due to <classifier> issue in <flattenDependencyMode>all</flattenDependencyMode> * remove unused imports * add more tests
|
Resolve #7576 |
Patch for https://issues.apache.org/jira/browse/MNG-6294