-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
New XTS integration tests #4928
Conversation
Build 455 is now running using a merge of 3b9539536bf0ff460f7344d9b495377f37fb9b04 |
Build 455 outcome was FAILURE using a merge of 3b9539536bf0ff460f7344d9b495377f37fb9b04 Failed tests
|
Looks like an intermittent failure to me: retest this please |
Build 465 is now running using a merge of 3b9539536bf0ff460f7344d9b495377f37fb9b04 |
.addPackage(EventLog.class.getPackage()) | ||
.addPackage(BaseFunctionalTest.class.getPackage()) | ||
|
||
.addAsResource("context-handlers.xml") // is this needed? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it is is still needed to setup the server-side handler chain. We are working on inferring this and adding automatically in the subsystem.
You may want to squash the commits as they all relate to the same issue. |
I think you need to remove MockSet and all it's usages. I don't think it has any active role in the tests? See my previous comment about how I think it got included. |
private static final Logger log = Logger.getLogger(ATClient.class); | ||
|
||
private static final String NODE0_ADDR = System.getProperty("node0", "localhost"); | ||
//TODO: parameterize this - /socket-binding-group=standard-sockets/socket-binding=http:read-attribute(name=port) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you need to do this? I agree you would need this for regular code, but for tests?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bstansberry Does the todo need addressing for tests?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, hard coding 8080 is fine.
The handling of NODE0_ADDR needs fixing though. The string needs to be passed through a routine like org.jboss.as.network.NetworkUtils.formatPossibleIpv6Address(...) before it can be used in the URL at L53 below. We commonly configure CI runs to bind node0 to an IPv6 address.
Build 465 outcome was SUCCESS using a merge of 3b9539536bf0ff460f7344d9b495377f37fb9b04 |
Build 209 is now running using a merge of 3b9539536bf0ff460f7344d9b495377f37fb9b04 |
Build 209 outcome was SUCCESS using a merge of 3b9539536bf0ff460f7344d9b495377f37fb9b04 |
@paulrobinson -- given the number of comments you've had on this, I'm not going to try and figure out when they've been satisfactorily addressed. So when this PR is ok with you, please be sure and add a comment explicitly approving it. Thanks! |
No problem @bstansberry,I'll keep an eye on it. I am on Holiday until the 26th August, so may not reply immediately. |
try { | ||
userTransaction = UserTransactionFactory.userTransaction(); | ||
String transactionId = userTransaction.transactionIdentifier(); | ||
System.out.println("RestaurantServiceAT transaction id =" + transactionId); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
system out an log? in same test?
Hi, thank you for the comments. @paulrobinson: I've removed MockSet object. Initially I thought that it is fine to "simulate" some business logic. And I've squashed the commits as you proposed. |
retest this please |
Build 258 is now running using a merge of dc021c1414bf84037a9de8069641f4eb4f206539 |
Build 258 outcome was SUCCESS using a merge of dc021c1414bf84037a9de8069641f4eb4f206539 |
@bstansberry All my comments have been satisfactorily addressed. |
@paulrobinson thanks So, whoever next merges a set of pulls (maybe me!!), this is good to go. |
.addAsWebInfResource(EmptyAsset.INSTANCE, ArchivePaths.create("beans.xml")) | ||
.addAsManifestResource(new StringAsset("Dependencies: org.jboss.xts,org.jboss.jts\n"), "MANIFEST.MF"); | ||
|
||
File testPackage = new File("/tmp", ARCHIVE_NAME + ".war"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This fails on windows for obvious reasons. Is this used anywhere in the test? If not, better just remove this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's the exception stacktrace for reference:
Caused by: org.jboss.shrinkwrap.api.exporter.ArchiveExportException: File could not be created: \tmp\wsba-coordinatorcompletition-test.war
at org.jboss.shrinkwrap.impl.base.exporter.AbstractStreamExporterImpl.getOutputStreamToFile(AbstractStreamExporterImpl.java:95)
at org.jboss.shrinkwrap.impl.base.exporter.AbstractStreamExporterImpl.exportTo(AbstractStreamExporterImpl.java:144)
at org.jboss.as.test.xts.wsba.coordinatorcompletion.client.BACoordinatorCompletionTestCase.createTestArchive(BACoordinatorCompletionTestCase.java:84)
... 59 more
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no, you are absolutely right. It has nothing to do in the code. That was my inattention which left the code at the place. I've checked it and I hope that there won't be more stuff like that. I removed that from the pull request. Sorry.
Build 310 outcome was FAILURE using a merge of 76207f7 Failed tests
|
the fails seems to me not to be connected with the pull request retest this please |
Build 312 outcome was FAILURE using a merge of 76207f7 Failed tests
|
merging |
merged |
Build 364 outcome was FAILURE using a merge of 76207f7 Failed tests
|
Changed tests based on narayana XTS tests. More details at https://issues.jboss.org/browse/JBQA-7126.