Skip to content
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

Update to fix the ArjunaJTA/jta tests to work under the codeCoverage #954

Merged
merged 3 commits into from Dec 8, 2015

Conversation

@jbosstm-bot

This comment has been minimized.

@jbosstm-bot

This comment has been minimized.

Copy link

commented Dec 2, 2015

Started testing this pull request with BLACKTIE profile on Windows: http://albany.eng.hst.ams2.redhat.com/job/btny-pulls-narayana-catelyn/999/

@jbosstm-bot

This comment has been minimized.

@jbosstm-bot

This comment has been minimized.

@zhfeng

This comment has been minimized.

Copy link
Contributor Author

commented Dec 3, 2015

The windows build fails due to the new maven changes the "tools/maven/bin/mvn.bat" to "mvn.cmd". This causes the build.bat does not work

@gytis

This comment has been minimized.

Copy link
Member

commented Dec 3, 2015

Hi Amos, I've raised https://issues.jboss.org/browse/JBTM-2576 for this.

@gytis

This comment has been minimized.

Copy link
Member

commented Dec 3, 2015

Should be all good now.

@zhfeng zhfeng force-pushed the zhfeng:codeCoverage branch from 0b3b19f to 60b7210 Dec 4, 2015
@jbosstm-bot

This comment has been minimized.

Copy link

commented Dec 4, 2015

Started testing this pull request with BLACKTIE profile on Windows: http://albany.eng.hst.ams2.redhat.com/job/btny-pulls-narayana-catelyn/1004/

@jbosstm-bot

This comment has been minimized.

Copy link

commented Dec 4, 2015

BLACKTIE profile tests passed on Windows - Job complete http://albany.eng.hst.ams2.redhat.com/job/btny-pulls-narayana-catelyn/1004/

@zhfeng

This comment has been minimized.

@@ -61,7 +61,7 @@ public final void setup() throws Exception {

resetPropertiesFile = System
.getProperty("com.arjuna.ats.arjuna.common.propertiesFile");
if (resetPropertiesFile == null) {
if (resetPropertiesFile == null || !resetPropertiesFile.equals("commitmarkableresourcejbossts-properties.xml")) {

This comment has been minimized.

Copy link
@mmusgrov

mmusgrov Dec 7, 2015

Contributor

The name and location of the properties file is no longer configurable. If that is the intention then just hard code it directly and don't do any getProperty calls or tests:

System.setProperty("com.arjuna.ats.arjuna.common.propertiesFile", "commitmarkableresourcejbossts-properties.xml");

This comment has been minimized.

Copy link
@zhfeng

zhfeng Dec 7, 2015

Author Contributor

So that means in the tearDown() it should just clear this property
System.clearProperty("com.arjuna.ats.arjuna.common.propertiesFile");

This comment has been minimized.

Copy link
@mmusgrov

mmusgrov Dec 7, 2015

Contributor

The updated code in the PR that deals with this property does not seem to do anything other than ensure that the property com.arjuna.ats.arjuna.common.propertiesFile takes the value "commitmarkableresourcejbossts-properties.xml" and this can be achieved by setting the property without any tests.

It is good practice to clean up in tear down but that seems unrelated to my comment.

This comment has been minimized.

Copy link
@zhfeng

zhfeng Dec 7, 2015

Author Contributor

It looks like in the pom.xml the maven-surefire-plugin sets the system property

<systemProperties>
       <property>
                <name>com.arjuna.ats.arjuna.common.propertiesFile</name>
                <value>jbossts-properties.xml</value>
      </property>
 </systemProperties>

It would be better to reset to "jbossts-properties.xml" or this is the default value now ?

This comment has been minimized.

Copy link
@mmusgrov

mmusgrov Dec 7, 2015

Contributor

No it wouldn't be better to reset to "jbossts-properties.xml" because this is just a test.

All I am saying is that after your changes the new code is semantically identical to:

System.setProperty("com.arjuna.ats.arjuna.common.propertiesFile", "commitmarkableresourcejbossts-properties.xml");

in which case you should use that since the changes you have introduced add needless complexity.

On the other hand, if you think your changes are not semantically identical to a single System.setProperty then please explain why (I may not have understood how your changes work)

This comment has been minimized.

Copy link
@tomjenkinson

tomjenkinson Dec 7, 2015

Contributor

Just to comment to the origin of this property manipulation. The properties file is set to jbossts-properties.xml in the surefire config and as this test needs to set it to commitmarkableresourcejbossts-properties.xml, it caches the original value and then is able to revert it to the value from the surefire config at the end of the test. I can't quite see why the diff added a check for resetPropertiesFile.equals("commitmarkableresourcejbossts-properties.xml") to maintain that behaviour but perhaps I missed something and if so please can you explain why the proposed change to this line required?

This comment has been minimized.

Copy link
@mmusgrov

mmusgrov Dec 7, 2015

Contributor

Ah right, thanks for the clarification Tom. So, Amos, I think it is safe to remove the resetPropertiesFile.equals ... check, do you agree?

This comment has been minimized.

Copy link
@zhfeng

zhfeng Dec 8, 2015

Author Contributor

Thanks Tom, and Mike, I agree with you that it should only set the property without any check.
The original check is if (resetPropertiesFile == null) that could have a problem when running the codeCoverage as the surefire config set it as "jbossts-peroperties.xml". That's why I added the equals check but it looks like redundant.

@@ -78,7 +78,7 @@ public final void setup() throws Exception {
namingBeanImpl.start();

resetPropertiesFile = System.getProperty("com.arjuna.ats.arjuna.common.propertiesFile");
if (resetPropertiesFile == null) {
if (resetPropertiesFile == null || !resetPropertiesFile.equals("commitmarkableresourcejbossts-properties.xml")) {

This comment has been minimized.

Copy link
@mmusgrov

mmusgrov Dec 7, 2015

Contributor

Same comment as above

@mmusgrov mmusgrov assigned zhfeng and unassigned mmusgrov Dec 7, 2015
@zhfeng zhfeng force-pushed the zhfeng:codeCoverage branch from 60b7210 to 762bc12 Dec 8, 2015
@zhfeng

This comment has been minimized.

Copy link
Contributor Author

commented Dec 8, 2015

Mike, I just update the fix and can you review it again ?

@zhfeng zhfeng assigned mmusgrov and unassigned zhfeng Dec 8, 2015
@jbosstm-bot

This comment has been minimized.

Copy link

commented Dec 8, 2015

Started testing this pull request with BLACKTIE profile on Windows: http://albany.eng.hst.ams2.redhat.com/job/btny-pulls-narayana-catelyn/1007/

@jbosstm-bot

This comment has been minimized.

Copy link

commented Dec 8, 2015

BLACKTIE profile tests passed on Windows - Job complete http://albany.eng.hst.ams2.redhat.com/job/btny-pulls-narayana-catelyn/1007/

@mmusgrov

This comment has been minimized.

Copy link
Contributor

commented Dec 8, 2015

The changes for JBTM-2573 are still wrong. The bug report does not explain what the issue is, in fact the description is empty (this is a mandatory field) so it would help me if you could update the JIRA explaining what the issue is about and why the code fails (since it looked like the code you changed already did what @tomjenkinson intended). Maybe the JIRA relates to a specific test but that is not described anywhere.

@zhfeng

This comment has been minimized.

Copy link
Contributor Author

commented Dec 8, 2015

Oh, sorry that it's my mistake and I will update the description soon.

@zhfeng

This comment has been minimized.

Copy link
Contributor Author

commented Dec 8, 2015

yeah, exactly. My mistake to remove the line of getProperty.

@zhfeng zhfeng force-pushed the zhfeng:codeCoverage branch from 762bc12 to 7ac263b Dec 8, 2015
@jbosstm-bot

This comment has been minimized.

Copy link

commented Dec 8, 2015

Started testing this pull request with BLACKTIE profile on Windows: http://albany.eng.hst.ams2.redhat.com/job/btny-pulls-narayana-catelyn/1009/

@mmusgrov mmusgrov assigned zhfeng and unassigned mmusgrov Dec 8, 2015
@mmusgrov

This comment has been minimized.

Copy link
Contributor

commented Dec 8, 2015

The fix looks good, thanks

@jbosstm-bot

This comment has been minimized.

Copy link

commented Dec 8, 2015

BLACKTIE profile tests passed on Windows - Job complete http://albany.eng.hst.ams2.redhat.com/job/btny-pulls-narayana-catelyn/1009/

@zhfeng zhfeng merged commit 7ac263b into jbosstm:master Dec 8, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.