New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

properties in arquillian.xml take precedence over system properties #816

Closed
Ladicek opened this Issue Oct 5, 2017 · 4 comments

Comments

Projects
None yet
2 participants
@Ladicek
Contributor

Ladicek commented Oct 5, 2017

Issue Overview

I have arquillian.xml looking like this:

<arquillian>
  <extension qualifier="openshift">
    <property name="namespace.use.current">true</property>
    <property name="env.init.enabled">true</property>
  </extension>
</arquillian>

(Ignore the env.init.enabled bit, I know it defaults to true, I include it just to make it clear that I depend on that behavior.)

I intentionally set namespace.use.current to true here so that the tests default to running in current namespace. But sometimes, I want Cube to create the temporary itest-xxxx namespace and use it, and I'm invoking Maven with -Dnamespace.use.current=false to do that.

Expected Behaviour

A temporary namespace is created and used.

Current Behaviour

Current namespace is used even though I specify -Dnamespace.use.current=false.

Steps To Reproduce
  1. git clone -b arquillian-cube http://github.com/Ladicek/wfswarm-rest-http/
  2. cd wfswarm-rest-http
  3. mvn clean verify -Popenshift,openshift-it -Dnamespace.use.current=false -DenableImageStreamDetection=true
Additional Information

This is caused by the org.arquillian.cube.impl.util.ConfigUtil implementation. The first method's javadoc actually says

The lookup order is system > env > map > defaultValue.

but the implementation doesn't conform to that javadoc.

I can send a PR, it's a pretty simple change, but I want to confirm first that it's really a bug. (I think it is.)

@lordofthejars

This comment has been minimized.

Show comment
Hide comment
@lordofthejars

lordofthejars Oct 6, 2017

Member

Humm I am not sure since surefire also does some strange things when propagating system properties and this features comes from arquillian core, and I am sure it has been widely tested and used, but for just in case try this:

First of all and IIRC maybe @MatousJobanek can help on this the -D property should -Dopenshift.namespace.use.current=false since I think that you need to qualify the system property.

If does not work it is better to try this:

Change <property name="namespace.use.current">true</property> to <property name="namespace.use.current">${useCurrentNamespace:true}</property> Now notice that what I saying is use true if system property useCurrentNamespace is not set otherwise use the value.

If still does not work then in your test try to do a sysout of the system property, to check if System Property is correctly bypassed by surefire.

Member

lordofthejars commented Oct 6, 2017

Humm I am not sure since surefire also does some strange things when propagating system properties and this features comes from arquillian core, and I am sure it has been widely tested and used, but for just in case try this:

First of all and IIRC maybe @MatousJobanek can help on this the -D property should -Dopenshift.namespace.use.current=false since I think that you need to qualify the system property.

If does not work it is better to try this:

Change <property name="namespace.use.current">true</property> to <property name="namespace.use.current">${useCurrentNamespace:true}</property> Now notice that what I saying is use true if system property useCurrentNamespace is not set otherwise use the value.

If still does not work then in your test try to do a sysout of the system property, to check if System Property is correctly bypassed by surefire.

@Ladicek

This comment has been minimized.

Show comment
Hide comment
@Ladicek

Ladicek Oct 6, 2017

Contributor

Actually the ConfigUtil class comes from Cube, and its implementation is inconsistent with its own Javadoc. After fixing that class, -Dnamespace.use.current=false works just fine. But I understand that this should be consistent with Arquillian itself -- I'm not sure right now what's the expected behavior there.

BTW, passing -DenableImageStreamDetection=true works A-OK, so I'd expect -Dnamespace.use.current=false to work as well. I tried -Dopenshift.namespace.use.current=false, but to no avail.

Your suggestion to add indirection through another system property obviously works perfectly.

Contributor

Ladicek commented Oct 6, 2017

Actually the ConfigUtil class comes from Cube, and its implementation is inconsistent with its own Javadoc. After fixing that class, -Dnamespace.use.current=false works just fine. But I understand that this should be consistent with Arquillian itself -- I'm not sure right now what's the expected behavior there.

BTW, passing -DenableImageStreamDetection=true works A-OK, so I'd expect -Dnamespace.use.current=false to work as well. I tried -Dopenshift.namespace.use.current=false, but to no avail.

Your suggestion to add indirection through another system property obviously works perfectly.

@lordofthejars

This comment has been minimized.

Show comment
Hide comment
@lordofthejars

lordofthejars Oct 6, 2017

Member

but arquillian.xml can work as well. But it might be a bug on Cube as you mention that overrides this, would you provide a PR with the fix? For what you mention it seems that you've already modified the code.

Member

lordofthejars commented Oct 6, 2017

but arquillian.xml can work as well. But it might be a bug on Cube as you mention that overrides this, would you provide a PR with the fix? For what you mention it seems that you've already modified the code.

@Ladicek

This comment has been minimized.

Show comment
Hide comment
@Ladicek

Ladicek Oct 6, 2017

Contributor

Indeed I changed the code to try if I can fix it in Cube, my changes are here: #818

Contributor

Ladicek commented Oct 6, 2017

Indeed I changed the code to try if I can fix it in Cube, my changes are here: #818

@lordofthejars lordofthejars closed this in #818 Oct 6, 2017

@lordofthejars lordofthejars added this to the 1.9.1 milestone Oct 13, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment