Skip to content

fixing Stomp::testSendWithHeartBeats#87

Closed
clebertsuconic wants to merge 1 commit intoapache:masterfrom
clebertsuconic:stomp-master
Closed

fixing Stomp::testSendWithHeartBeats#87
clebertsuconic wants to merge 1 commit intoapache:masterfrom
clebertsuconic:stomp-master

Conversation

@clebertsuconic
Copy link
Copy Markdown
Contributor

No description provided.

@clebertsuconic
Copy link
Copy Markdown
Contributor Author

The test got broken at ebcf0c0

@scop Can you check my fix please testSendWithHeartBeats was hanging as it was receiving (NULL) instead of a just null package. (or something like that).. run the test without this fix and you would see.

@asfbot
Copy link
Copy Markdown

asfbot commented Jul 20, 2015

ActiveMQ-Artemis-PR-Build #537 FAILURE
Looks like there's a problem with this pull request

@scop
Copy link
Copy Markdown
Contributor

scop commented Jul 20, 2015

Actually I have another PR coming up which I believe would address this issue in a better way.

I suppose I haven't had enough coverage on my local test suite runs, I've done mvn -Pdev test at top level. I now did the same with -Pfast-tests but don't see the failure with that either. If I want to run the full set of say STOMP tests, what command in which dir would you recommend? What did you run in order to see the failure?

@clebertsuconic
Copy link
Copy Markdown
Contributor Author

I've tested stomp/* using my IDE. Perhaps we should create a profile for the stomp testsuite? or refactor the tests under protocols/stomp? (that last part is a bit tricky due to the super classes).

@clebertsuconic
Copy link
Copy Markdown
Contributor Author

@scop so what you want to do, should I close this and wait your PR? or should I fix my PR (that failed here) and merge this?

@scop
Copy link
Copy Markdown
Contributor

scop commented Jul 20, 2015

If you want the failure gone ASAP, go ahead and push your fix to it, I'll adapt mine later.

One the other hand I can get my PR submitted tonight provided I can get the test suite to run some sane way. I'm an Eclipse user and I guess I found the place where to run the desired test suites, problem is that when I do right-click "Run as" -> "JUnit test" for the stomp package in integration-tests I get a lot of errors about classes not found, some ExceptionInInitializerErrors due to Caused by: java.lang.IllegalArgumentException: Invalid logger interface org.apache.activemq.artemis.tests.integration.IntegrationTestLogger (implementation not found in sun.misc.Launcher$AppClassLoader@409a44d6) etc, so I suppose my setup needs some changes, unfortunately I'm not sure what...

@clebertsuconic
Copy link
Copy Markdown
Contributor Author

We need a way to run a set of tests.

I will wait until tomorrow.

I haven't used Eclipse in a while.. but I'm sure others did. (I have been using Intelij)

@clebertsuconic
Copy link
Copy Markdown
Contributor Author

@scop I will send a commit to the master fixing the test, feel free to improve the fix... I need to get some build results.

@clebertsuconic
Copy link
Copy Markdown
Contributor Author

@scop build started at https://builds.apache.org/job/ActiveMQ-Artemis-Nightly-Regression-Test/202/

As I said, please feel free to improve the fix.

@scop
Copy link
Copy Markdown
Contributor

scop commented Jul 21, 2015

After some ugly local hacks I got the tests to run. I see testSendAndReceiveWithHeartBeats hanging (did you mean that instead of testSendWithHeartBeats?), but I cannot seem to find a revision in which it would /not/ hang. It hangs for me also with your fix. I've backed down to commit 6a59443 but it too shows the same problems for me: the while (frame != null) never becomes false, the connection keeps receiving heartbeat frames forever after the ten message frames it correctly receives.

I'm afraid I'm at loss here.

@clebertsuconic
Copy link
Copy Markdown
Contributor Author

@scop TBH those were the reasons why I stopped using eclipse. Too hard to make it happen.

With Idea I just open a maven project and it works.

I'm not saying it's impossible, but it's certainly harder. You should probably ask for help on the dev-list

@clebertsuconic
Copy link
Copy Markdown
Contributor Author

or switch to idea ;)

@scop
Copy link
Copy Markdown
Contributor

scop commented Jul 21, 2015

Maybe sometime. But just to confirm: does the fix you committed for this fix the hang for you?

Another thing about your fix: it reverts the duplicate content-length change from PR #86, for no apparent (at least to me) reason. Was that on purpose?

@clebertsuconic
Copy link
Copy Markdown
Contributor Author

sorry about that.. just fixed it

d0k1 pushed a commit to d0k1/activemq-artemis that referenced this pull request Dec 21, 2015
clebertsuconic added a commit to clebertsuconic/artemis that referenced this pull request Jun 3, 2016
ARTEMIS-539 fixing default address on client as well
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants