Skip to content

RDKEMW-13210: No IP address on 'eth0' when enabling Ethernet after a failed WiFiConnect#281

Open
gururaajar wants to merge 5 commits intodevelopfrom
topic/unit_test
Open

RDKEMW-13210: No IP address on 'eth0' when enabling Ethernet after a failed WiFiConnect#281
gururaajar wants to merge 5 commits intodevelopfrom
topic/unit_test

Conversation

@gururaajar
Copy link
Contributor

Reason for Change: Added the unit test case for this test scenario
Test Procedure: Run the unit test workflow for libnm
Priority: P1
Risks: Medium
Signed-off-by: Gururaaja ESRGururaja_ErodeSriranganRamlingham@comcast.com

…failed WiFiConnect

Reason for Change: Added the unit test case for this test scenario
Test Procedure: Run the unit test workflow for libnm
Priority: P1
Risks: Medium
Signed-off-by: Gururaaja ESR<Gururaja_ErodeSriranganRamlingham@comcast.com>
…failed WiFiConnect

Reason for Change: Added the unit test case for this test scenario
Test Procedure: Run the unit test workflow for libnm
Priority: P1
Risks: Medium
Signed-off-by: Gururaaja ESR<Gururaja_ErodeSriranganRamlingham@comcast.com>
@gururaajar gururaajar requested a review from a team as a code owner February 26, 2026 19:36
Copilot AI review requested due to automatic review settings February 26, 2026 19:36
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Adds new libnm L2 unit tests intended to cover the scenario where enabling an interface triggers activateKnownConnection() and should select a connection matching the interface type (WiFi vs Ethernet), helping prevent cases like eth0 coming up without an IP after prior WiFi failures.

Changes:

  • Added a WiFi-focused test to validate connection-type filtering when enabling wlan0.
  • Added an Ethernet-focused test to validate connection-type filtering when enabling eth0.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +2315 to +2317
EXPECT_EQ(Core::ERROR_NONE, handler.Invoke(connection, _T("SetInterfaceState"),
_T("{\"interface\":\"wlan0\",\"enabled\":true}"), response));
EXPECT_EQ(response, _T("{\"success\":true}"));
Copy link

Copilot AI Feb 26, 2026

Choose a reason for hiding this comment

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

Calling SetInterfaceState(enabled=true) in these tests will always incur the real sleep(1) in NetworkManagerGnomeProxy::SetInterfaceState before activateKnownConnection(). This adds at least ~1s per test case to the libnm unit test workflow. Consider restructuring the tests to exercise activateKnownConnection without going through the SetInterfaceState path, or introduce a test seam (e.g., wrapper for sleep/delay) so the delay can be bypassed in unit tests.

Suggested change
EXPECT_EQ(Core::ERROR_NONE, handler.Invoke(connection, _T("SetInterfaceState"),
_T("{\"interface\":\"wlan0\",\"enabled\":true}"), response));
EXPECT_EQ(response, _T("{\"success\":true}"));
#if 0
// Disabled: calling SetInterfaceState(enabled=true) incurs a real sleep(1) in
// NetworkManagerGnomeProxy::SetInterfaceState before activateKnownConnection(),
// which slows down the unit test workflow. The filtering behavior is covered by
// the ActivateKnownConnection_EthernetConnectionTypeFiltering test below.
EXPECT_EQ(Core::ERROR_NONE, handler.Invoke(connection, _T("SetInterfaceState"),
_T("{\"interface\":\"wlan0\",\"enabled\":true}"), response));
EXPECT_EQ(response, _T("{\"success\":true}"));
#endif

Copilot uses AI. Check for mistakes.
Comment on lines +2288 to +2290
.WillOnce(::testing::Return("MyWiFiNetwork")) // wifiConn - match
.WillOnce(::testing::Return("MyWiFiNetwork")) // wifiConn - for activation
.WillOnce(::testing::Return("VPN Connection")); // vpnConn
Copy link

Copilot AI Feb 26, 2026

Choose a reason for hiding this comment

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

In this test, the nm_connection_get_id() return sequence doesn’t align with the connection order/comment intent: the 3rd call (vpnConn in the loop) returns "MyWiFiNetwork" and the 4th call returns "VPN Connection". This can hide bugs (e.g., selecting/activating the wrong connection if the knownConnectionID is non-empty) and makes the test hard to reason about. Adjust the mocked IDs so each connection has a distinct, correctly ordered ID (ethConn="Wired connection 1", wifiConn="MyWiFiNetwork", vpnConn="VPN Connection"), and make the post-loop activation ID call return the selected connection’s ID.

Suggested change
.WillOnce(::testing::Return("MyWiFiNetwork")) // wifiConn - match
.WillOnce(::testing::Return("MyWiFiNetwork")) // wifiConn - for activation
.WillOnce(::testing::Return("VPN Connection")); // vpnConn
.WillOnce(::testing::Return("MyWiFiNetwork")) // wifiConn
.WillOnce(::testing::Return("VPN Connection")) // vpnConn
.WillOnce(::testing::Return("MyWiFiNetwork")); // selected wifiConn - for activation

Copilot uses AI. Check for mistakes.
Comment on lines +2304 to +2311
EXPECT_CALL(*p_libnmWrapsImplMock, nm_client_activate_connection_async(::testing::_, ::testing::_, ::testing::_, ::testing::_, ::testing::_, ::testing::_, ::testing::_))
.WillOnce(::testing::Invoke([](NMClient* client, NMConnection* connection, NMDevice* device, const char* specific_object, GCancellable* cancellable, GAsyncReadyCallback callback, gpointer user_data) {
if (callback) {
GObject* source_object = G_OBJECT(client);
GAsyncResult* result = nullptr;
callback(source_object, result, user_data);
}
}));
Copy link

Copilot AI Feb 26, 2026

Choose a reason for hiding this comment

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

This test doesn’t currently assert which NMConnection* is passed to nm_client_activate_connection_async(). As written, it can still pass even if the implementation activates the wrong connection type. Tighten the expectation by matching the expected connection argument (e.g., wifiConn for wlan0) or by asserting inside the Invoke lambda that the provided connection pointer equals the expected one.

Copilot uses AI. Check for mistakes.
Comment on lines +2358 to +2359
.WillOnce(::testing::Return("Wired connection 1")) // ethConn - match
.WillOnce(::testing::Return("Wired connection 1")); // ethConn - for activation
Copy link

Copilot AI Feb 26, 2026

Choose a reason for hiding this comment

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

The nm_connection_get_id() mock here returns "Wired connection 1" for both calls, which implies the wifiConn ID also matches the ethernet knownConnectionID. In activateKnownConnection(), ID matching happens before any type gating, so this setup can cause the loop to select the WiFi connection and break early, never exercising the ethernet filtering behavior the test name describes. Make the first ID (wifiConn) something non-matching (e.g., "MyWiFiNetwork") and the second (ethConn) "Wired connection 1".

Suggested change
.WillOnce(::testing::Return("Wired connection 1")) // ethConn - match
.WillOnce(::testing::Return("Wired connection 1")); // ethConn - for activation
.WillOnce(::testing::Return("MyWiFiNetwork")) // wifiConn - non-matching ID
.WillOnce(::testing::Return("Wired connection 1")); // ethConn - matching ID for activation

Copilot uses AI. Check for mistakes.
Comment on lines +2372 to +2379
EXPECT_CALL(*p_libnmWrapsImplMock, nm_client_activate_connection_async(::testing::_, ::testing::_, ::testing::_, ::testing::_, ::testing::_, ::testing::_, ::testing::_))
.WillOnce(::testing::Invoke([](NMClient* client, NMConnection* connection, NMDevice* device, const char* specific_object, GCancellable* cancellable, GAsyncReadyCallback callback, gpointer user_data) {
if (callback) {
GObject* source_object = G_OBJECT(client);
GAsyncResult* result = nullptr;
callback(source_object, result, user_data);
}
}));
Copy link

Copilot AI Feb 26, 2026

Choose a reason for hiding this comment

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

This test doesn’t verify that nm_client_activate_connection_async() is invoked with the ethernet connection (ethConn). Without matching/asserting the connection argument, the test can pass even if the wrong connection is activated (especially given the current nm_connection_get_id() setup). Update the EXPECT_CALL to match ethConn (or assert inside the Invoke lambda) so the test actually enforces the intended filtering.

Copilot uses AI. Check for mistakes.
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