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

Fixed ServerLowLevel tests not running #580

Merged
merged 2 commits into from
May 23, 2021

Conversation

hylkevds
Copy link
Collaborator

JUnit only runs tests in classes that have a name that ends in Test,
the class in question had a name that ended in Tests, with an added s.
As a result, the tests in this class were never executed. This renames
the class to not have the added s.

@hylkevds
Copy link
Collaborator Author

The first commit fixes the tests not running, the second commit adds a test for #573.
So the builds should all fail after the second commit, and be fixed again by PR #576.

JUnit only runs tests in classes that have a name that ends in Test,
the class in question had a name that ended in Tests, with an added s.
As a result, the tests in this class were never executed. This renames
the class to not have the added s.
Copy link
Collaborator

@andsel andsel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @hylkevds the test is usefull, I would suggest some changes to be more explicit why some magic numbers are the values they are.

subscriber.subscribe(topic, 1, (String topic1, org.eclipse.paho.client.mqttv3.MqttMessage message) -> {
if (isFirst.getAndSet(false)) {
// wait to trigger resending PUBLISH
TimeUnit.SECONDS.sleep(12);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

12 seconds is more then 2 times of resend interval (

private static final int FLIGHT_BEFORE_RESEND_MS = 5_000;
) this direct linkage with this interval should be more explicit, so:

  • move from private to package private or even public that constant and reuse here as FLIGHT_BEFORE_RESEND_MS * 2

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree using the constant is much nicer, but Session would also have to be made public for that to work...
Another option is to make a separate class for constants.
What would you prefer?

publisher.connect();

AtomicBoolean isFirst = new AtomicBoolean(true);
CountDownLatch countDownLatch = new CountDownLatch(2);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The countdown latch initialized to 2 is redundant. It should be initialized to 1 and countDown() only when it reaches the second publish.

It could be done even more straightforward with Awaitility, for example:

AtomicBoolean isFirst = new AtomicBoolean(true);
AtomicBoolean receivedPublish = new AtomicBoolean(false);
subscriber.subscribe(topic, 1, (String topic1, org.eclipse.paho.client.mqttv3.MqttMessage message) -> {
  if (isFirst.getAndSet(false)) {
    TimeUnit.MILLISECONDS.sleep(FLIGHT_BEFORE_RESEND_MS * 2);
  } else {
    receivedPublish.set(true); 
  }
});
publisher.publish(topic, "hello".getBytes(), 1, false);

Awaitility.await()
    .atMost(FLIGHT_BEFORE_RESEND_MS * 4, TimeUnit.MILLISECONDS)
    .pollInterval(FLIGHT_BEFORE_RESEND_MS, TimeUnit.MILLISECONDS)
    .untill(receivedPublish::get, equalTo(true));
   

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is better is we move those two constants into broker/src/main/java/io/moquette/BrokerConstants.java wihtou changing visibility of Session class

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I'll do that, and clean up the test.

@hylkevds hylkevds force-pushed the fix_lowLevelTest_NotRunning branch from 69e4114 to cbc31c5 Compare May 22, 2021 16:47
@hylkevds hylkevds force-pushed the fix_lowLevelTest_NotRunning branch from cbc31c5 to ba0f9eb Compare May 22, 2021 18:23
@hylkevds hylkevds requested a review from andsel May 22, 2021 18:27
Copy link
Collaborator

@andsel andsel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@andsel andsel merged commit 4e29483 into moquette-io:master May 23, 2021
@hylkevds hylkevds deleted the fix_lowLevelTest_NotRunning branch May 23, 2021 20:54
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.

2 participants