You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@creadur.apache.org by po...@apache.org on 2014/08/11 07:46:11 UTC

svn commit: r1617205 - /creadur/rat/trunk/apache-rat-plugin/src/test/java/org/apache/rat/mp/RatCheckMojoTest.java

Author: pottlinger
Date: Mon Aug 11 05:46:11 2014
New Revision: 1617205

URL: http://svn.apache.org/r1617205
Log:
RAT-168 Tests don't check CLI usage

-Ensuring that project itself and its list of remote ArtifactRepos are notNull in integration tests.

Modified:
    creadur/rat/trunk/apache-rat-plugin/src/test/java/org/apache/rat/mp/RatCheckMojoTest.java

Modified: creadur/rat/trunk/apache-rat-plugin/src/test/java/org/apache/rat/mp/RatCheckMojoTest.java
URL: http://svn.apache.org/viewvc/creadur/rat/trunk/apache-rat-plugin/src/test/java/org/apache/rat/mp/RatCheckMojoTest.java?rev=1617205&r1=1617204&r2=1617205&view=diff
==============================================================================
--- creadur/rat/trunk/apache-rat-plugin/src/test/java/org/apache/rat/mp/RatCheckMojoTest.java (original)
+++ creadur/rat/trunk/apache-rat-plugin/src/test/java/org/apache/rat/mp/RatCheckMojoTest.java Mon Aug 11 05:46:11 2014
@@ -177,6 +177,9 @@ public class RatCheckMojoTest extends Ab
             }
         };
         setVariableValueToObject( mojo, "project", project );
+        assertNotNull("RAT-168: Problem in test setup - you are missing a project in your mojo.", project);
+        assertNotNull("RAT-168: The mojo is missing its MavenProject, which will result in an NPE during rat runs.", mojo.getProject());
+        assertNotNull("RAT-168: No artifactRepos found, which will result in an NPE during rat runs.", project.getRemoteArtifactRepositories());
         if (mojo instanceof RatReportMojo)
         {
             setVariableValueToObject( mojo, "localRepository", newArtifactRepository() );

Re: svn commit: r1617205 - /creadur/rat/trunk/apache-rat-plugin/src/test/java/org/apache/rat/mp/RatCheckMojoTest.java

Posted by Dennis Lundberg <de...@apache.org>.
On Mon, Aug 11, 2014 at 12:38 PM, sebb <se...@gmail.com> wrote:
> On 11 August 2014 11:17, P. Ottlinger <po...@aiki-it.de> wrote:
>> Hi Sebb,
>>
>>
>> On 2014-08-11 12:09, sebb wrote:
>>>
>>> AFAICT the test does not detect the error.
>>
>>
>> yes, that's why I was asking for proposals on how to test it ;-)
>>
>>
>>> I think the problem is that the tests are run in a different environment.
>>>
>>> There probably needs to be an IT instead to run the code directly.
>>> And this commit should probably be reverted, as it does not add anything.
>>
>>
>> I see your point, but my commit ensures that during IT the project and its
>> artifact lists is not null.
>
> OK, then drop the references to the JIRA, as the checks aren't relevant to it.
>
>> Obviously my test patch didn't catch the original NPE, but my checkin fixed
>> the issue for your setup, didn't it?
>
> Yes, the NPE disappeared.
>
>> I tried some stackoverflowing but didn't find anything useful. Many people
>> complain about the low testability of maven plugins.
>
> I suspect the issue is documentation.
> [I have had a quick look, and there seems to be nothing that explains
> how to build IT tests in proper detail.]

Here's an overview of the different strategies for testing Maven
Plugins, for future reference:
http://maven.apache.org/guides/development/guide-testing-releases.html

Over at the Maven project we do most of the testing of Maven Plugin as
integration tests using maven-invoker-plugin.

> As an experiment, I tried changing
>
> apache-rat-plugin/src/test/invoker/it1/invoker.properties
>
> to run the rat goal rather than check
>
> This produced the NPE when I reverted the fix.
> However, the test also fails with the fix, presumably because the goal
> output is different.
>
> I think there needs to be another IT with extra tests, but I have not
> created any such items.
> It should be possible to copy/adapt another IT test, but getting that
> working properly might not be easy owing to the fragmented and
> incomplete documentation.
>
> I may give it a try, but no promises!
>
>> Thanks
>> Phil
>>
>>



-- 
Dennis Lundberg

Re: svn commit: r1617205 - /creadur/rat/trunk/apache-rat-plugin/src/test/java/org/apache/rat/mp/RatCheckMojoTest.java

Posted by Dennis Lundberg <de...@apache.org>.
On Mon, Aug 11, 2014 at 2:15 PM, sebb <se...@gmail.com> wrote:
> On 11 August 2014 12:59, Dennis Lundberg <de...@apache.org> wrote:
>> On Mon, Aug 11, 2014 at 12:38 PM, sebb <se...@gmail.com> wrote:
>>> On 11 August 2014 11:17, P. Ottlinger <po...@aiki-it.de> wrote:
>>>> Hi Sebb,
>>>>
>>>>
>>>> On 2014-08-11 12:09, sebb wrote:
>>>>>
>>>>> AFAICT the test does not detect the error.
>>>>
>>>>
>>>> yes, that's why I was asking for proposals on how to test it ;-)
>>>>
>>>>
>>>>> I think the problem is that the tests are run in a different environment.
>>>>>
>>>>> There probably needs to be an IT instead to run the code directly.
>>>>> And this commit should probably be reverted, as it does not add anything.
>>>>
>>>>
>>>> I see your point, but my commit ensures that during IT the project and its
>>>> artifact lists is not null.
>>>
>>> OK, then drop the references to the JIRA, as the checks aren't relevant to it.
>>>
>>>> Obviously my test patch didn't catch the original NPE, but my checkin fixed
>>>> the issue for your setup, didn't it?
>>>
>>> Yes, the NPE disappeared.
>>>
>>>> I tried some stackoverflowing but didn't find anything useful. Many people
>>>> complain about the low testability of maven plugins.
>>>
>>> I suspect the issue is documentation.
>>> [I have had a quick look, and there seems to be nothing that explains
>>> how to build IT tests in proper detail.]
>>>
>>> As an experiment, I tried changing
>>>
>>> apache-rat-plugin/src/test/invoker/it1/invoker.properties
>>>
>>> to run the rat goal rather than check
>>>
>>> This produced the NPE when I reverted the fix.
>>> However, the test also fails with the fix, presumably because the goal
>>> output is different.
>>>
>>> I think there needs to be another IT with extra tests, but I have not
>>> created any such items.
>>> It should be possible to copy/adapt another IT test, but getting that
>>> working properly might not be easy owing to the fragmented and
>>> incomplete documentation.
>>
>> I agree. It's better to have many small ITs that each test one thing.
>
> +1
>
>> Let me have a look at the ITs in general, and adding one that catches
>> whatever is causing problems. I've created a decent amount of Maven
>> Plugin ITs in my day.
>
> Great - maybe we can a README to the IT tree to explain how to add new tests?
> Also point to the Maven docs that describe the layout and processing.
>
> For example, I can see that there are test/ and test/invoker
> sub-folders that relate to each other.
> However, it is not clear how the shared files under test/ and
> test/invoker should be used.
> In particular, the files under java - are these used for all it tests?
> If so. how does one allow for different test conditions?

I moved things around a bit to make it clearer which parts belong
together. The integration tests are now under src/it and the unit
style tests are now under src/test/java and src/test/resources. These
locations are the standard locations for these kinds of tests in a
Maven project, see
http://maven.apache.org/guides/introduction/introduction-to-the-standard-directory-layout.html

I hope that there is now a clear separation between unit and integration tests.

>
>>> I may give it a try, but no promises!
>
> I added it4_RAT-168
>
> But note that it2 and it3 don't seem to be run currently - RAT-169
> It would be good to fix that.
>
>>>
>>>> Thanks
>>>> Phil
>>>>
>>>>
>>
>>
>>
>> --
>> Dennis Lundberg



-- 
Dennis Lundberg

Re: svn commit: r1617205 - /creadur/rat/trunk/apache-rat-plugin/src/test/java/org/apache/rat/mp/RatCheckMojoTest.java

Posted by sebb <se...@gmail.com>.
On 11 August 2014 13:15, sebb <se...@gmail.com> wrote:
> On 11 August 2014 12:59, Dennis Lundberg <de...@apache.org> wrote:
>> On Mon, Aug 11, 2014 at 12:38 PM, sebb <se...@gmail.com> wrote:
>>> On 11 August 2014 11:17, P. Ottlinger <po...@aiki-it.de> wrote:
>>>> Hi Sebb,
>>>>
>>>>
>>>> On 2014-08-11 12:09, sebb wrote:
>>>>>
>>>>> AFAICT the test does not detect the error.
>>>>
>>>>
>>>> yes, that's why I was asking for proposals on how to test it ;-)
>>>>
>>>>
>>>>> I think the problem is that the tests are run in a different environment.
>>>>>
>>>>> There probably needs to be an IT instead to run the code directly.
>>>>> And this commit should probably be reverted, as it does not add anything.
>>>>
>>>>
>>>> I see your point, but my commit ensures that during IT the project and its
>>>> artifact lists is not null.
>>>
>>> OK, then drop the references to the JIRA, as the checks aren't relevant to it.
>>>
>>>> Obviously my test patch didn't catch the original NPE, but my checkin fixed
>>>> the issue for your setup, didn't it?
>>>
>>> Yes, the NPE disappeared.
>>>
>>>> I tried some stackoverflowing but didn't find anything useful. Many people
>>>> complain about the low testability of maven plugins.
>>>
>>> I suspect the issue is documentation.
>>> [I have had a quick look, and there seems to be nothing that explains
>>> how to build IT tests in proper detail.]
>>>
>>> As an experiment, I tried changing
>>>
>>> apache-rat-plugin/src/test/invoker/it1/invoker.properties
>>>
>>> to run the rat goal rather than check
>>>
>>> This produced the NPE when I reverted the fix.
>>> However, the test also fails with the fix, presumably because the goal
>>> output is different.
>>>
>>> I think there needs to be another IT with extra tests, but I have not
>>> created any such items.
>>> It should be possible to copy/adapt another IT test, but getting that
>>> working properly might not be easy owing to the fragmented and
>>> incomplete documentation.
>>
>> I agree. It's better to have many small ITs that each test one thing.
>
> +1
>
>> Let me have a look at the ITs in general, and adding one that catches
>> whatever is causing problems. I've created a decent amount of Maven
>> Plugin ITs in my day.
>
> Great - maybe we can a README to the IT tree to explain how to add new tests?
> Also point to the Maven docs that describe the layout and processing.
>
> For example, I can see that there are test/ and test/invoker
> sub-folders that relate to each other.

Looks like test/invoker/it1 and test/it1 have nothing to do with each other.
They just happen to have the same name

> However, it is not clear how the shared files under test/ and
> test/invoker should be used.

test/invoker seems to be used for the IT tests only, not JUnit tests.

> In particular, the files under java - are these used for all it tests?

The java file specifically targets it[1-3]/pom.xml in separate test methods

> If so. how does one allow for different test conditions?

They are checked in the relevant test method.

>>> I may give it a try, but no promises!
>
> I added it4_RAT-168
>
> But note that it2 and it3 don't seem to be run currently - RAT-169
> It would be good to fix that.

Not a problem - they are part of RatCheckMojoTest

>>>
>>>> Thanks
>>>> Phil
>>>>
>>>>
>>
>>
>>
>> --
>> Dennis Lundberg

Re: svn commit: r1617205 - /creadur/rat/trunk/apache-rat-plugin/src/test/java/org/apache/rat/mp/RatCheckMojoTest.java

Posted by "P. Ottlinger" <po...@aiki-it.de>.
Hi Sebb, hi Dennis,

thanks for your help in identifying that RAT's testing can be improved.

Am 11.08.2014 um 14:15 schrieb sebb:
>> Let me have a look at the ITs in general, and adding one that catches
>> > whatever is causing problems. I've created a decent amount of Maven
>> > Plugin ITs in my day.
> Great - maybe we can a README to the IT tree to explain how to add new tests?
> Also point to the Maven docs that describe the layout and processing.
> 
> For example, I can see that there are test/ and test/invoker
> sub-folders that relate to each other.
> However, it is not clear how the shared files under test/ and
> test/invoker should be used.
> In particular, the files under java - are these used for all it tests?
> If so. how does one allow for different test conditions?
> 
>>> >> I may give it a try, but no promises!
> I added it4_RAT-168
> 
> But note that it2 and it3 don't seem to be run currently - RAT-169
> It would be good to fix that.

Following your discussion I've extracted some static testHelper methods
in order to be able to see tests more clearly.

Hope this is okay and we can continue to create a new 0.11 RC.

What do you think?

Phil

Re: svn commit: r1617205 - /creadur/rat/trunk/apache-rat-plugin/src/test/java/org/apache/rat/mp/RatCheckMojoTest.java

Posted by sebb <se...@gmail.com>.
On 11 August 2014 12:59, Dennis Lundberg <de...@apache.org> wrote:
> On Mon, Aug 11, 2014 at 12:38 PM, sebb <se...@gmail.com> wrote:
>> On 11 August 2014 11:17, P. Ottlinger <po...@aiki-it.de> wrote:
>>> Hi Sebb,
>>>
>>>
>>> On 2014-08-11 12:09, sebb wrote:
>>>>
>>>> AFAICT the test does not detect the error.
>>>
>>>
>>> yes, that's why I was asking for proposals on how to test it ;-)
>>>
>>>
>>>> I think the problem is that the tests are run in a different environment.
>>>>
>>>> There probably needs to be an IT instead to run the code directly.
>>>> And this commit should probably be reverted, as it does not add anything.
>>>
>>>
>>> I see your point, but my commit ensures that during IT the project and its
>>> artifact lists is not null.
>>
>> OK, then drop the references to the JIRA, as the checks aren't relevant to it.
>>
>>> Obviously my test patch didn't catch the original NPE, but my checkin fixed
>>> the issue for your setup, didn't it?
>>
>> Yes, the NPE disappeared.
>>
>>> I tried some stackoverflowing but didn't find anything useful. Many people
>>> complain about the low testability of maven plugins.
>>
>> I suspect the issue is documentation.
>> [I have had a quick look, and there seems to be nothing that explains
>> how to build IT tests in proper detail.]
>>
>> As an experiment, I tried changing
>>
>> apache-rat-plugin/src/test/invoker/it1/invoker.properties
>>
>> to run the rat goal rather than check
>>
>> This produced the NPE when I reverted the fix.
>> However, the test also fails with the fix, presumably because the goal
>> output is different.
>>
>> I think there needs to be another IT with extra tests, but I have not
>> created any such items.
>> It should be possible to copy/adapt another IT test, but getting that
>> working properly might not be easy owing to the fragmented and
>> incomplete documentation.
>
> I agree. It's better to have many small ITs that each test one thing.

+1

> Let me have a look at the ITs in general, and adding one that catches
> whatever is causing problems. I've created a decent amount of Maven
> Plugin ITs in my day.

Great - maybe we can a README to the IT tree to explain how to add new tests?
Also point to the Maven docs that describe the layout and processing.

For example, I can see that there are test/ and test/invoker
sub-folders that relate to each other.
However, it is not clear how the shared files under test/ and
test/invoker should be used.
In particular, the files under java - are these used for all it tests?
If so. how does one allow for different test conditions?

>> I may give it a try, but no promises!

I added it4_RAT-168

But note that it2 and it3 don't seem to be run currently - RAT-169
It would be good to fix that.

>>
>>> Thanks
>>> Phil
>>>
>>>
>
>
>
> --
> Dennis Lundberg

Re: svn commit: r1617205 - /creadur/rat/trunk/apache-rat-plugin/src/test/java/org/apache/rat/mp/RatCheckMojoTest.java

Posted by Dennis Lundberg <de...@apache.org>.
On Mon, Aug 11, 2014 at 12:38 PM, sebb <se...@gmail.com> wrote:
> On 11 August 2014 11:17, P. Ottlinger <po...@aiki-it.de> wrote:
>> Hi Sebb,
>>
>>
>> On 2014-08-11 12:09, sebb wrote:
>>>
>>> AFAICT the test does not detect the error.
>>
>>
>> yes, that's why I was asking for proposals on how to test it ;-)
>>
>>
>>> I think the problem is that the tests are run in a different environment.
>>>
>>> There probably needs to be an IT instead to run the code directly.
>>> And this commit should probably be reverted, as it does not add anything.
>>
>>
>> I see your point, but my commit ensures that during IT the project and its
>> artifact lists is not null.
>
> OK, then drop the references to the JIRA, as the checks aren't relevant to it.
>
>> Obviously my test patch didn't catch the original NPE, but my checkin fixed
>> the issue for your setup, didn't it?
>
> Yes, the NPE disappeared.
>
>> I tried some stackoverflowing but didn't find anything useful. Many people
>> complain about the low testability of maven plugins.
>
> I suspect the issue is documentation.
> [I have had a quick look, and there seems to be nothing that explains
> how to build IT tests in proper detail.]
>
> As an experiment, I tried changing
>
> apache-rat-plugin/src/test/invoker/it1/invoker.properties
>
> to run the rat goal rather than check
>
> This produced the NPE when I reverted the fix.
> However, the test also fails with the fix, presumably because the goal
> output is different.
>
> I think there needs to be another IT with extra tests, but I have not
> created any such items.
> It should be possible to copy/adapt another IT test, but getting that
> working properly might not be easy owing to the fragmented and
> incomplete documentation.

I agree. It's better to have many small ITs that each test one thing.
Let me have a look at the ITs in general, and adding one that catches
whatever is causing problems. I've created a decent amount of Maven
Plugin ITs in my day.

> I may give it a try, but no promises!
>
>> Thanks
>> Phil
>>
>>



-- 
Dennis Lundberg

Re: svn commit: r1617205 - /creadur/rat/trunk/apache-rat-plugin/src/test/java/org/apache/rat/mp/RatCheckMojoTest.java

Posted by sebb <se...@gmail.com>.
On 11 August 2014 11:17, P. Ottlinger <po...@aiki-it.de> wrote:
> Hi Sebb,
>
>
> On 2014-08-11 12:09, sebb wrote:
>>
>> AFAICT the test does not detect the error.
>
>
> yes, that's why I was asking for proposals on how to test it ;-)
>
>
>> I think the problem is that the tests are run in a different environment.
>>
>> There probably needs to be an IT instead to run the code directly.
>> And this commit should probably be reverted, as it does not add anything.
>
>
> I see your point, but my commit ensures that during IT the project and its
> artifact lists is not null.

OK, then drop the references to the JIRA, as the checks aren't relevant to it.

> Obviously my test patch didn't catch the original NPE, but my checkin fixed
> the issue for your setup, didn't it?

Yes, the NPE disappeared.

> I tried some stackoverflowing but didn't find anything useful. Many people
> complain about the low testability of maven plugins.

I suspect the issue is documentation.
[I have had a quick look, and there seems to be nothing that explains
how to build IT tests in proper detail.]

As an experiment, I tried changing

apache-rat-plugin/src/test/invoker/it1/invoker.properties

to run the rat goal rather than check

This produced the NPE when I reverted the fix.
However, the test also fails with the fix, presumably because the goal
output is different.

I think there needs to be another IT with extra tests, but I have not
created any such items.
It should be possible to copy/adapt another IT test, but getting that
working properly might not be easy owing to the fragmented and
incomplete documentation.

I may give it a try, but no promises!

> Thanks
> Phil
>
>

Re: svn commit: r1617205 - /creadur/rat/trunk/apache-rat-plugin/src/test/java/org/apache/rat/mp/RatCheckMojoTest.java

Posted by "P. Ottlinger" <po...@aiki-it.de>.
Hi Sebb,

On 2014-08-11 12:09, sebb wrote:
> AFAICT the test does not detect the error.

yes, that's why I was asking for proposals on how to test it ;-)

> I think the problem is that the tests are run in a different 
> environment.
> 
> There probably needs to be an IT instead to run the code directly.
> And this commit should probably be reverted, as it does not add 
> anything.

I see your point, but my commit ensures that during IT the project and 
its artifact lists is not null.

Obviously my test patch didn't catch the original NPE, but my checkin 
fixed the issue for your setup, didn't it?

I tried some stackoverflowing but didn't find anything useful. Many 
people complain about the low testability of maven plugins.

Thanks
Phil

Re: svn commit: r1617205 - /creadur/rat/trunk/apache-rat-plugin/src/test/java/org/apache/rat/mp/RatCheckMojoTest.java

Posted by sebb <se...@gmail.com>.
On 11 August 2014 06:46,  <po...@apache.org> wrote:
> Author: pottlinger
> Date: Mon Aug 11 05:46:11 2014
> New Revision: 1617205
>
> URL: http://svn.apache.org/r1617205
> Log:
> RAT-168 Tests don't check CLI usage
>
> -Ensuring that project itself and its list of remote ArtifactRepos are notNull in integration tests.

AFAICT the test does not detect the error.

I tried reverting to the @Parameter annotation and the test still succeeded.

I think the problem is that the tests are run in a different environment.

There probably needs to be an IT instead to run the code directly.
And this commit should probably be reverted, as it does not add anything.

> Modified:
>     creadur/rat/trunk/apache-rat-plugin/src/test/java/org/apache/rat/mp/RatCheckMojoTest.java
>
> Modified: creadur/rat/trunk/apache-rat-plugin/src/test/java/org/apache/rat/mp/RatCheckMojoTest.java
> URL: http://svn.apache.org/viewvc/creadur/rat/trunk/apache-rat-plugin/src/test/java/org/apache/rat/mp/RatCheckMojoTest.java?rev=1617205&r1=1617204&r2=1617205&view=diff
> ==============================================================================
> --- creadur/rat/trunk/apache-rat-plugin/src/test/java/org/apache/rat/mp/RatCheckMojoTest.java (original)
> +++ creadur/rat/trunk/apache-rat-plugin/src/test/java/org/apache/rat/mp/RatCheckMojoTest.java Mon Aug 11 05:46:11 2014
> @@ -177,6 +177,9 @@ public class RatCheckMojoTest extends Ab
>              }
>          };
>          setVariableValueToObject( mojo, "project", project );
> +        assertNotNull("RAT-168: Problem in test setup - you are missing a project in your mojo.", project);
> +        assertNotNull("RAT-168: The mojo is missing its MavenProject, which will result in an NPE during rat runs.", mojo.getProject());
> +        assertNotNull("RAT-168: No artifactRepos found, which will result in an NPE during rat runs.", project.getRemoteArtifactRepositories());
>          if (mojo instanceof RatReportMojo)
>          {
>              setVariableValueToObject( mojo, "localRepository", newArtifactRepository() );
>
>