SUREFIRE-491 copying sys props to forked process

[dfabulich]

The sad tale of SUREFIRE-491 began when I tried to fix SUREFIRE-121.

http://jira.codehaus.org/browse/SUREFIRE-491
http://jira.codehaus.org/browse/SUREFIRE-121

The request seemed innocent enough.  Wouldn't it be cool if you could pass
system properties to your tests, like this?

   mvn clean test -Dbrowser=firefox

Apparently this used to work in an earlier version of Surefire, even.
What could go wrong?

Well, a lot actually, because these days Surefire runs the tests in a
forked process by default; passing system properties to the forked process
means copying all system properties to the child.  I knew this was
risky...

http://www.nabble.com/passing-system-properties-to-forked-test-td13947630.html

I even included a comment in the code: "Is this wise?"
http://svn.apache.org/viewvc/maven/surefire/trunk/maven-surefire-plugin/src/main/java/org/apache/maven/plugin/surefire/SurefirePlugin.java?r1=598217&r2=598216&pathrev=598217

It turns out that it was NOT wise.  What if the child is running a
different version of Java?  Copying all system properties means copying
the java.specification.version into the child, so the child thinks it's
running the same version as the parent.  It gets even sillier; we copy
java.class.path from the parent to the child, so while the classpath may
be correct, the system property is not.

At this point, I'm tempted to resolve SUREFIRE-491 by breaking
SUREFIRE-121 again and marking it "Won't Fix", preventing the user from
passing system properties to forked tests simply by specifying them on the
command line.  It's convenient, but I just can't see any way to support it
safely.

Any other suggestions before I just roll ahead with this?

-Dan

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@...
For additional commands, e-mail: dev-help@...

[brettporter]

I thought I had commented on this at some point, but I don't really  
remember.

You're plan of action sounds correct if there's no way around it. You  
might however look into the changes John made in 2.0.9 to separate CLI  
properties from existing system properties - maybe we can just support  
it under 2.0.9 using these?

- Brett

On 02/05/2008, at 12:18 PM, Dan Fabulich wrote:

>
> The sad tale of SUREFIRE-491 began when I tried to fix SUREFIRE-121.
>
> http://jira.codehaus.org/browse/SUREFIRE-491
> http://jira.codehaus.org/browse/SUREFIRE-121
>
> The request seemed innocent enough.  Wouldn't it be cool if you  
> could pass system properties to your tests, like this?
>
>  mvn clean test -Dbrowser=firefox
>
> Apparently this used to work in an earlier version of Surefire,  
> even. What could go wrong?
>
> Well, a lot actually, because these days Surefire runs the tests in  
> a forked process by default; passing system properties to the forked  
> process means copying all system properties to the child.  I knew  
> this was risky...
>
> http://www.nabble.com/passing-system-properties-to-forked-test-td13947630.html
>
> I even included a comment in the code: "Is this wise?"
> http://svn.apache.org/viewvc/maven/surefire/trunk/maven-surefire-plugin/src/main/java/org/apache/maven/plugin/surefire/SurefirePlugin.java?r1=598217&r2=598216&pathrev=598217
>
> It turns out that it was NOT wise.  What if the child is running a  
> different version of Java?  Copying all system properties means  
> copying the java.specification.version into the child, so the child  
> thinks it's running the same version as the parent.  It gets even  
> sillier; we copy java.class.path from the parent to the child, so  
> while the classpath may be correct, the system property is not.
>
> At this point, I'm tempted to resolve SUREFIRE-491 by breaking  
> SUREFIRE-121 again and marking it "Won't Fix", preventing the user  
> from passing system properties to forked tests simply by specifying  
> them on the command line.  It's convenient, but I just can't see any  
> way to support it safely.
>
> Any other suggestions before I just roll ahead with this?
>
> -Dan
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@...
> For additional commands, e-mail: dev-help@...
>

--
Brett Porter
brett@...
http://blogs.exist.com/bporter/


---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@...
For additional commands, e-mail: dev-help@...

[dfabulich]

Brett Porter wrote:

> I thought I had commented on this at some point, but I don't really remember.
>
> You're plan of action sounds correct if there's no way around it. You might
> however look into the changes John made in 2.0.9 to separate CLI properties
> from existing system properties - maybe we can just support it under 2.0.9
> using these?

Hmm.  Good suggestion... it looks like in 2.0.8 the user properties were
just being dumped into the execution properties together with the system
properties; now in 2.0.9 they're carried forward to the
MavenProjectBuilder where they're used for model interpolation... but then
we don't make them available as a map for plugin use.

Instead of "Won't Fix", I'll mark SUREFIRE-121 Future, I guess. :-)

-Dan



---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@...
For additional commands, e-mail: dev-help@...

[mihobson]

You could always namespace off system properties that should be passed
to the forked process, e.g. -Dtest.foo=bar would pass in -Dfoo=bar.

Mark

2008/5/2 Dan Fabulich <dan@...>:

> Brett Porter wrote:
>
>
> > I thought I had commented on this at some point, but I don't really
> remember.
> >
> > You're plan of action sounds correct if there's no way around it. You
> might however look into the changes John made in 2.0.9 to separate CLI
> properties from existing system properties - maybe we can just support it
> under 2.0.9 using these?
> >
>
>  Hmm.  Good suggestion... it looks like in 2.0.8 the user properties were
> just being dumped into the execution properties together with the system
> properties; now in 2.0.9 they're carried forward to the MavenProjectBuilder
> where they're used for model interpolation... but then we don't make them
> available as a map for plugin use.
>
>  Instead of "Won't Fix", I'll mark SUREFIRE-121 Future, I guess. :-)
>
>
>
>  -Dan
>
>
>
>  ---------------------------------------------------------------------
>  To unsubscribe, e-mail: dev-unsubscribe@...
>  For additional commands, e-mail: dev-help@...
>
>

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@...
For additional commands, e-mail: dev-help@...

[Aaron Digulla]

Dan Fabulich schrieb:

> The sad tale of SUREFIRE-491 began when I tried to fix SUREFIRE-121.
>
> http://jira.codehaus.org/browse/SUREFIRE-491
> http://jira.codehaus.org/browse/SUREFIRE-121
>
> The request seemed innocent enough.  Wouldn't it be cool if you could
> pass system properties to your tests, like this?
>
>   mvn clean test -Dbrowser=firefox
>
> Apparently this used to work in an earlier version of Surefire, even.
> What could go wrong?
> [...]
> Any other suggestions before I just roll ahead with this?

IMHO, the correct fix would be to collect command line properties
somewhere in the model and copy only *those* to the child process.

That might be more work but as you said, it's a) very convenient and b)
the current implementation just isn't doing what the user expects.

Regards,

--
Aaron "Optimizer" Digulla a.k.a. Philmann Dark
"It's not the universe that's limited, it's our imagination.
Follow me and I'll show you something beyond the limits."
http://darkviews.blogspot.com/          http://www.pdark.de/

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@...
For additional commands, e-mail: dev-help@...