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.

Sign up for GitHub

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

fix test failing because of timeout that aren't testing timing #375

Merged
merged 7 commits into from
Dec 14, 2020
Merged

fix test failing because of timeout that aren't testing timing #375

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
b88c57a
fix testDisconnectReconnectCount
scottf Dec 13, 2020
b205278
fix timing problems on non timing tests
scottf Dec 13, 2020
5bd649b
fix timing problems on non timing tests 2
scottf Dec 13, 2020
874a560
aroc macros more descriptive
scottf Dec 14, 2020
3c9bb72
more macro use it 1 of 3
scottf Dec 14, 2020
af728fc
more macro use it sleep
scottf Dec 14, 2020
0374297
aroc naming
scottf Dec 14, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
144 changes: 46 additions & 98 deletions src/test/java/io/nats/client/ConnectTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,31 +13,26 @@

package io.nats.client;

import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertFalse;
import static org.junit.jupiter.api.Assertions.assertNotNull;
import static org.junit.jupiter.api.Assertions.assertThrows;
import static org.junit.jupiter.api.Assertions.assertTrue;
import io.nats.client.ConnectionListener.Events;
import io.nats.client.NatsServerProtocolMock.ExitAt;
import org.junit.jupiter.api.Test;

import java.io.IOException;
import java.time.Duration;
import java.util.concurrent.TimeUnit;

import org.junit.jupiter.api.Test;

import io.nats.client.ConnectionListener.Events;
import io.nats.client.NatsServerProtocolMock.ExitAt;
import static io.nats.client.impl.TestMacros.*;
import static org.junit.jupiter.api.Assertions.*;

public class ConnectTests {
@Test
public void testDefaultConnection() throws IOException, InterruptedException {
try (NatsTestServer ts = new NatsTestServer(Options.DEFAULT_PORT, false)) {
Connection nc = Nats.connect();
try {
assertTrue(Connection.Status.CONNECTED == nc.getStatus(), "Connected Status");
assertConnected(nc);
} finally {
nc.close();
assertTrue(Connection.Status.CLOSED == nc.getStatus(), "Closed Status");
closeThenAssertClosed(nc);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I noticed it was the same thing over and over with no variation. "Macros" aren't really a java thing, just something I like. I think they add description to the tests and simplify routine scaffolding code.

}
}
}
Expand All @@ -47,10 +42,9 @@ public void testConnection() throws IOException, InterruptedException {
try (NatsTestServer ts = new NatsTestServer(false)) {
Connection nc = Nats.connect(ts.getURI());
try {
assertTrue(Connection.Status.CONNECTED == nc.getStatus(), "Connected Status");
assertConnected(nc);
} finally {
nc.close();
assertTrue(Connection.Status.CLOSED == nc.getStatus(), "Closed Status");
closeThenAssertClosed(nc);
}
}
}
Expand All @@ -61,10 +55,9 @@ public void testConnectionWithOptions() throws IOException, InterruptedException
Options options = new Options.Builder().server(ts.getURI()).build();
Connection nc = Nats.connect(options);
try {
assertTrue(Connection.Status.CONNECTED == nc.getStatus(), "Connected Status");
assertConnected(nc);
} finally {
nc.close();
assertTrue(Connection.Status.CLOSED == nc.getStatus(), "Closed Status");
closeThenAssertClosed(nc);
}
}
}
Expand All @@ -74,10 +67,9 @@ public void testFullFakeConnect() throws IOException, InterruptedException {
try (NatsServerProtocolMock ts = new NatsServerProtocolMock(ExitAt.NO_EXIT)) {
Connection nc = Nats.connect(ts.getURI());
try {
assertTrue(Connection.Status.CONNECTED == nc.getStatus(), "Connected Status");
assertConnected(nc);
} finally {
nc.close();
assertTrue(Connection.Status.CLOSED == nc.getStatus(), "Closed Status");
closeThenAssertClosed(nc);
}
}
}
Expand All @@ -88,10 +80,9 @@ public void testFullFakeConnectWithTabs() throws IOException, InterruptedExcepti
ts.useTabs();
Connection nc = Nats.connect(ts.getURI());
try {
assertTrue(Connection.Status.CONNECTED == nc.getStatus(), "Connected Status");
assertConnected(nc);
} finally {
nc.close();
assertTrue(Connection.Status.CLOSED == nc.getStatus(), "Closed Status");
closeThenAssertClosed(nc);
}
}
}
Expand All @@ -105,10 +96,7 @@ public void testConnectExitBeforeInfo() {
try {
nc = Nats.connect(opt);
} finally {
if (nc != null) {
nc.close();
assertTrue(Connection.Status.CLOSED == nc.getStatus(), "Closed Status");
}
closeThenAssertClosed(nc);
}
}
});
Expand All @@ -123,10 +111,7 @@ public void testConnectExitAfterInfo() {
try {
nc = Nats.connect(opt);
} finally {
if (nc != null) {
nc.close();
assertTrue(Connection.Status.CLOSED == nc.getStatus(), "Closed Status");
}
closeThenAssertClosed(nc);
}
}
});
Expand All @@ -141,10 +126,7 @@ public void testConnectExitAfterConnect() {
try {
nc = Nats.connect(opt);
} finally {
if (nc != null) {
nc.close();
assertTrue(Connection.Status.CLOSED == nc.getStatus(), "Closed Status");
}
closeThenAssertClosed(nc);
}
}
});
Expand All @@ -159,10 +141,7 @@ public void testConnectExitAfterPing() {
try {
nc = Nats.connect(opt);
} finally {
if (nc != null) {
nc.close();
assertTrue(Connection.Status.CLOSED == nc.getStatus(), "Closed Status");
}
closeThenAssertClosed(nc);
}
}
});
Expand All @@ -176,10 +155,9 @@ public void testConnectionFailureWithFallback() throws IOException, InterruptedE
Options options = new Options.Builder().connectionTimeout(Duration.ofSeconds(5)).server(fake.getURI()).server(ts.getURI()).build();
Connection nc = Nats.connect(options);
try {
assertEquals(Connection.Status.CONNECTED, nc.getStatus(), "Connected Status");
assertConnected(nc);
} finally {
nc.close();
assertTrue(Connection.Status.CLOSED == nc.getStatus(), "Closed Status");
closeThenAssertClosed(nc);
}
}
}
Expand All @@ -190,10 +168,9 @@ public void testConnectWithConfig() throws IOException, InterruptedException {
try (NatsTestServer ts = new NatsTestServer("src/test/resources/simple.conf", false)) {
Connection nc = Nats.connect(ts.getURI());
try {
assertTrue(Connection.Status.CONNECTED == nc.getStatus(), "Connected Status");
assertConnected(nc);
} finally {
nc.close();
assertTrue(Connection.Status.CLOSED == nc.getStatus(), "Closed Status");
closeThenAssertClosed(nc);
}
}
}
Expand All @@ -204,10 +181,9 @@ public void testConnectWithCommas() throws IOException, InterruptedException {
try (NatsTestServer ts2 = new NatsTestServer(false)) {
Connection nc = Nats.connect(ts1.getURI() + "," + ts2.getURI());
try {
assertTrue(Connection.Status.CONNECTED == nc.getStatus(), "Connected Status");
assertConnected(nc);
} finally {
nc.close();
assertTrue(Connection.Status.CLOSED == nc.getStatus(), "Closed Status");
closeThenAssertClosed(nc);
}
}
}
Expand All @@ -224,16 +200,15 @@ public void testConnectRandomize() throws IOException, InterruptedException {
for (int i=0; i < 10; i++) {
Connection nc = Nats.connect(ts1.getURI() + "," + ts2.getURI());
try {
assertTrue(Connection.Status.CONNECTED == nc.getStatus(), "Connected Status");
assertConnected(nc);

if (nc.getConnectedUrl().equals(ts1.getURI())) {
one++;
} else {
two++;
}
} finally {
nc.close();
assertTrue(Connection.Status.CLOSED == nc.getStatus(), "Closed Status");
closeThenAssertClosed(nc);
}
}

Expand All @@ -256,21 +231,20 @@ public void testConnectNoRandomize() throws IOException, InterruptedException {
Options options = new Options.Builder().noRandomize().servers(servers).build();
Connection nc = Nats.connect(options);
try {
assertTrue(Connection.Status.CONNECTED == nc.getStatus(), "Connected Status");
assertConnected(nc);

if (nc.getConnectedUrl().equals(ts1.getURI())) {
one++;
} else {
two++;
}
} finally {
nc.close();
assertTrue(Connection.Status.CLOSED == nc.getStatus(), "Closed Status");
closeThenAssertClosed(nc);
}
}

assertTrue(one == 10, "always got one");
assertTrue(two == 0, "never got two");
assertEquals(one, 10, "always got one");
assertEquals(two, 0, "never got two");
Copy link
Contributor Author

Choose a reason for hiding this comment

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

nitty for sure, but again how the assertions were designed.

Copy link
Member

Choose a reason for hiding this comment

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

I see linter complaints about these... thanks.

}
}
}
Expand All @@ -285,10 +259,7 @@ public void testFailWithMissingLineFeedAfterInfo() {
try {
nc = Nats.connect(options);
} finally {
if (nc != null) {
nc.close();
assertTrue(Connection.Status.CLOSED == nc.getStatus(), "Closed Status");
}
closeThenAssertClosed(nc);
}
}
});
Expand All @@ -304,10 +275,7 @@ public void testFailWithStuffAfterInitialInfo() {
try {
nc = Nats.connect(options);
} finally {
if (nc != null) {
nc.close();
assertTrue(Connection.Status.CLOSED == nc.getStatus(), "Closed Status");
}
closeThenAssertClosed(nc);
}
}
});
Expand All @@ -324,10 +292,7 @@ public void testFailWrongInitialInfoOP() {
try {
nc = Nats.connect(options);
} finally {
if (nc != null) {
nc.close();
assertTrue(Connection.Status.CLOSED == nc.getStatus(), "Closed Status");
}
closeThenAssertClosed(nc);
}
}
});
Expand All @@ -343,10 +308,7 @@ public void testIncompleteInitialInfo() {
try {
nc = Nats.connect(options);
} finally {
if (nc != null) {
nc.close();
assertTrue(Connection.Status.CLOSED == nc.getStatus(), "Closed Status");
}
closeThenAssertClosed(nc);
}
}
});
Expand All @@ -371,12 +333,9 @@ public void testAsyncConnection() throws IOException, InterruptedException {
try {
nc = handler.getConnection();
assertNotNull(nc);
assertTrue(Connection.Status.CONNECTED == nc.getStatus(), "Connected Status");
assertConnected(nc);
} finally {
if (nc != null) {
nc.close();
}
assertTrue(Connection.Status.CLOSED == nc.getStatus(), "Closed Status");
closeThenAssertClosed(nc);
}
}
}
Expand All @@ -396,26 +355,17 @@ public void testAsyncConnectionWithReconnect() throws IOException, InterruptedEx
try {
Nats.connectAsynchronously(options, true);

// No server at this point, let it fail and try to start over
try {
Thread.sleep(1000);
} catch (Exception exp) {

}
sleep(5000); // No server at this point, let it fail and try to start over
Copy link
Contributor Author

Choose a reason for hiding this comment

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

sleep macro just cleaner.

Copy link
Member

Choose a reason for hiding this comment

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

could be a bit more deterministic and use options.getConnectionTimeout() + 100 in case the default timeout changes.


nc = handler.getConnection(); // will be disconnected, but should be there
assertNotNull(nc);

handler.prepForStatusChange(Events.RECONNECTED);
try (NatsTestServer ts = new NatsTestServer(port, false)) {
handler.waitForStatusChange(5, TimeUnit.SECONDS);
assertTrue(Connection.Status.CONNECTED == nc.getStatus(), "Connected Status");
waitThenAssertConnected(nc, handler);
Copy link
Contributor Author

@scottf scottf Dec 14, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

This macro fixed the flappers that didn't allow enough time for the connection. With the macro it's consistent. Only used in tests where they are not testing the connection itself, but using the connection to test something. This version of the macro waits up to 5 seconds. There is another version that you can specify the wait.

}
} finally {
if (nc != null) {
nc.close();
assertTrue(Connection.Status.CLOSED == nc.getStatus(), "Closed Status");
}
closeThenAssertClosed(nc);
}
}

Expand Down Expand Up @@ -459,7 +409,7 @@ public void testConnectionTimeout() {
connectionTimeout(Duration.ofSeconds(2)). // 2 is also the default but explicit for test
build();
Connection nc = Nats.connect(options);
assertFalse(Connection.Status.CONNECTED == nc.getStatus(), "Connected Status");
assertNotSame(Connection.Status.CONNECTED, nc.getStatus(), "Connected Status");
}
});
}
Expand All @@ -474,10 +424,9 @@ public void testSlowConnectionNoTimeout() throws IOException, InterruptedExcepti
build();
Connection nc = Nats.connect(options);
try {
assertTrue(Connection.Status.CONNECTED == nc.getStatus(), "Connected Status");
assertConnected(nc);
} finally {
nc.close();
assertTrue(Connection.Status.CLOSED == nc.getStatus(), "Closed Status");
closeThenAssertClosed(nc);
}
}
}
Expand All @@ -491,16 +440,15 @@ public void testTimeCheckCoverage() throws IOException, InterruptedException {
build();
Connection nc = Nats.connect(options);
try {
assertTrue(Connection.Status.CONNECTED == nc.getStatus(), "Connected Status");
assertConnected(nc);
} finally {
nc.close();
assertTrue(Connection.Status.CLOSED == nc.getStatus(), "Closed Status");
closeThenAssertClosed(nc);
}
}
}

@Test
public void testConnectExceptionHasURLS() throws IOException, InterruptedException {
public void testConnectExceptionHasURLS() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

exception was never thrown

try {
Nats.connect("nats://testserver.notnats:4222, nats://testserver.alsonotnats:4223");
} catch (Exception e) {
Expand Down
Loading