Fix for bug ONOS-959: BgpSessionManagerTest unit test failure
Previously, when checking the winning BGP routes, we expect-and-wait until
the size of the table with the winning routes has certain size, and
then we verify whether it contains a particular value.
This is error-prone and time-sensitive. For example, if we are testing
whether a particular route from one BGP peer is replaced with the
same route from another BGP peer, the test might fail because the
table size doesn't change.
Modified the unit test to expect-and-wait until the expected route
is received.
Also, fixed some of the paragraph tags in some of the Javadocs.
Change-Id: Ia96dc7c412e78bbc9279dd935dec6919096adeb3
diff --git a/apps/sdnip/src/test/java/org/onosproject/sdnip/bgp/BgpSessionManagerTest.java b/apps/sdnip/src/test/java/org/onosproject/sdnip/bgp/BgpSessionManagerTest.java
index 45ee321..7ae8658 100644
--- a/apps/sdnip/src/test/java/org/onosproject/sdnip/bgp/BgpSessionManagerTest.java
+++ b/apps/sdnip/src/test/java/org/onosproject/sdnip/bgp/BgpSessionManagerTest.java
@@ -299,9 +299,10 @@
/**
* Gets BGP RIB-IN routes by waiting until they are received.
- * <p/>
+ * <p>
* NOTE: We keep checking once every 10ms the number of received routes,
* up to 5 seconds.
+ * </p>
*
* @param bgpSession the BGP session that is expected to receive the
* routes
@@ -328,9 +329,10 @@
/**
* Gets BGP merged routes by waiting until they are received.
- * <p/>
+ * <p>
* NOTE: We keep checking once every 10ms the number of received routes,
* up to 5 seconds.
+ * </p>
*
* @param expectedRoutes the expected number of routes
* @return the BGP Session Manager routes as received within the expected
@@ -354,12 +356,45 @@
}
/**
+ * Gets a merged BGP route by waiting until it is received.
+ * <p>
+ * NOTE: We keep checking once every 10ms whether the route is received,
+ * up to 5 seconds.
+ * </p>
+ *
+ * @param expectedRoute the expected route
+ * @return the merged BGP route if received within the expected time
+ * interval, otherwise null
+ */
+ private BgpRouteEntry waitForBgpRoute(BgpRouteEntry expectedRoute)
+ throws InterruptedException {
+ Collection<BgpRouteEntry> bgpRoutes =
+ bgpSessionManager.getBgpRoutes4();
+
+ final int maxChecks = 500; // Max wait of 5 seconds
+ for (int i = 0; i < maxChecks; i++) {
+ for (BgpRouteEntry bgpRouteEntry : bgpRoutes) {
+ if (bgpRouteEntry.equals(expectedRoute) &&
+ bgpRouteEntry.getBgpSession() ==
+ expectedRoute.getBgpSession()) {
+ return bgpRouteEntry;
+ }
+ }
+ Thread.sleep(10);
+ bgpRoutes = bgpSessionManager.getBgpRoutes4();
+ }
+
+ return null;
+ }
+
+ /**
* Tests that the BGP OPEN messages have been exchanged, followed by
* KEEPALIVE.
* <p>
* The BGP Peer opens the sessions and transmits OPEN Message, eventually
* followed by KEEPALIVE. The tested BGP listener should respond by
* OPEN Message, followed by KEEPALIVE.
+ * </p>
*
* @throws TestUtilsException TestUtils error
*/
@@ -406,6 +441,7 @@
* The BGP Peer opens the sessions and transmits OPEN Message, eventually
* followed by KEEPALIVE. The tested BGP listener should respond by
* OPEN Message, followed by KEEPALIVE.
+ * </p>
*
* @throws TestUtilsException TestUtils error
*/
@@ -523,7 +559,7 @@
DEFAULT_LOCAL_PREF);
bgpRouteEntry.setMultiExitDisc(DEFAULT_MULTI_EXIT_DISC);
assertThat(bgpRibIn1, hasBgpRouteEntry(bgpRouteEntry));
- assertThat(bgpRoutes, hasBgpRouteEntry(bgpRouteEntry));
+ assertThat(waitForBgpRoute(bgpRouteEntry), notNullValue());
//
bgpRouteEntry =
new BgpRouteEntry(bgpSession1,
@@ -534,7 +570,7 @@
DEFAULT_LOCAL_PREF);
bgpRouteEntry.setMultiExitDisc(DEFAULT_MULTI_EXIT_DISC);
assertThat(bgpRibIn1, hasBgpRouteEntry(bgpRouteEntry));
- assertThat(bgpRoutes, hasBgpRouteEntry(bgpRouteEntry));
+ assertThat(waitForBgpRoute(bgpRouteEntry), notNullValue());
//
bgpRouteEntry =
new BgpRouteEntry(bgpSession1,
@@ -545,7 +581,7 @@
DEFAULT_LOCAL_PREF);
bgpRouteEntry.setMultiExitDisc(DEFAULT_MULTI_EXIT_DISC);
assertThat(bgpRibIn1, hasBgpRouteEntry(bgpRouteEntry));
- assertThat(bgpRoutes, hasBgpRouteEntry(bgpRouteEntry));
+ assertThat(waitForBgpRoute(bgpRouteEntry), notNullValue());
//
bgpRouteEntry =
new BgpRouteEntry(bgpSession1,
@@ -556,7 +592,7 @@
DEFAULT_LOCAL_PREF);
bgpRouteEntry.setMultiExitDisc(DEFAULT_MULTI_EXIT_DISC);
assertThat(bgpRibIn1, hasBgpRouteEntry(bgpRouteEntry));
- assertThat(bgpRoutes, hasBgpRouteEntry(bgpRouteEntry));
+ assertThat(waitForBgpRoute(bgpRouteEntry), notNullValue());
//
bgpRouteEntry =
new BgpRouteEntry(bgpSession1,
@@ -567,7 +603,7 @@
DEFAULT_LOCAL_PREF);
bgpRouteEntry.setMultiExitDisc(DEFAULT_MULTI_EXIT_DISC);
assertThat(bgpRibIn1, hasBgpRouteEntry(bgpRouteEntry));
- assertThat(bgpRoutes, hasBgpRouteEntry(bgpRouteEntry));
+ assertThat(waitForBgpRoute(bgpRouteEntry), notNullValue());
//
// Delete some routes
@@ -602,7 +638,7 @@
DEFAULT_LOCAL_PREF);
bgpRouteEntry.setMultiExitDisc(DEFAULT_MULTI_EXIT_DISC);
assertThat(bgpRibIn1, hasBgpRouteEntry(bgpRouteEntry));
- assertThat(bgpRoutes, hasBgpRouteEntry(bgpRouteEntry));
+ assertThat(waitForBgpRoute(bgpRouteEntry), notNullValue());
//
bgpRouteEntry =
new BgpRouteEntry(bgpSession1,
@@ -613,7 +649,7 @@
DEFAULT_LOCAL_PREF);
bgpRouteEntry.setMultiExitDisc(DEFAULT_MULTI_EXIT_DISC);
assertThat(bgpRibIn1, hasBgpRouteEntry(bgpRouteEntry));
- assertThat(bgpRoutes, hasBgpRouteEntry(bgpRouteEntry));
+ assertThat(waitForBgpRoute(bgpRouteEntry), notNullValue());
//
bgpRouteEntry =
new BgpRouteEntry(bgpSession1,
@@ -624,7 +660,7 @@
DEFAULT_LOCAL_PREF);
bgpRouteEntry.setMultiExitDisc(DEFAULT_MULTI_EXIT_DISC);
assertThat(bgpRibIn1, hasBgpRouteEntry(bgpRouteEntry));
- assertThat(bgpRoutes, hasBgpRouteEntry(bgpRouteEntry));
+ assertThat(waitForBgpRoute(bgpRouteEntry), notNullValue());
// Close the channels and test there are no routes
@@ -703,7 +739,7 @@
BETTER_LOCAL_PREF);
bgpRouteEntry.setMultiExitDisc(DEFAULT_MULTI_EXIT_DISC);
assertThat(bgpRibIn2, hasBgpRouteEntry(bgpRouteEntry));
- assertThat(bgpRoutes, hasBgpRouteEntry(bgpRouteEntry));
+ assertThat(waitForBgpRoute(bgpRouteEntry), notNullValue());
//
// Add a route entry to Peer3 with a shorter AS path
@@ -737,7 +773,7 @@
BETTER_LOCAL_PREF);
bgpRouteEntry.setMultiExitDisc(DEFAULT_MULTI_EXIT_DISC);
assertThat(bgpRibIn3, hasBgpRouteEntry(bgpRouteEntry));
- assertThat(bgpRoutes, hasBgpRouteEntry(bgpRouteEntry));
+ assertThat(waitForBgpRoute(bgpRouteEntry), notNullValue());
//
// Cleanup in preparation for next test: delete old route entry from
@@ -793,7 +829,7 @@
BETTER_LOCAL_PREF);
bgpRouteEntry.setMultiExitDisc(BETTER_MULTI_EXIT_DISC);
assertThat(bgpRibIn2, hasBgpRouteEntry(bgpRouteEntry));
- assertThat(bgpRoutes, hasBgpRouteEntry(bgpRouteEntry));
+ assertThat(waitForBgpRoute(bgpRouteEntry), notNullValue());
//
// Add a route entry to Peer1 with a better (lower) BGP ID
@@ -828,7 +864,7 @@
BETTER_LOCAL_PREF);
bgpRouteEntry.setMultiExitDisc(BETTER_MULTI_EXIT_DISC);
assertThat(bgpRibIn1, hasBgpRouteEntry(bgpRouteEntry));
- assertThat(bgpRoutes, hasBgpRouteEntry(bgpRouteEntry));
+ assertThat(waitForBgpRoute(bgpRouteEntry), notNullValue());
// Close the channels and test there are no routes