chore: updating mocks for tests to test mender-connect JWT request handling #2108

SebOpsahl · 2024-05-02T17:00:09Z

This PR is made to change the mocks and testflow in tests to better fit the new implementation of the PR 👉 mendersoftware/mender-connect#134. This PR fixes it so mender-connect requests a new JWT token on its own, so its not dependent on mender-update to do so. Previously FetchJWTToken() wasn’t called which wouldn’t let it call the JWT token independently, which made it dependent on mender-update. The proposed fix is using FetchJWTToken() to fetch the token, to then for WaitForJwtTokenStateChange() to retrieve it in it’s waiting. The old mocks and test flow for the tests seemed to be more tailored to test the functionality with GetJWTToken(). This PR proposes a change in the mocks and test flow to better test the new implementation.

Changelog: None

Ticket: MEN-6877

mender-test-bot · 2024-05-02T17:00:25Z

@SebOpsahl, Let me know if you want to start the integration pipeline by mentioning me and the command "start pipeline".

my commands and options

You can trigger a pipeline on multiple prs with:

mentioning me and start pipeline --pr mender/127 --pr mender-connect/255

You can start a fast pipeline, disabling full integration tests with:

mentioning me and start pipeline --fast

You can trigger GitHub->GitLab branch sync with:

mentioning me and sync

You can cherry pick to a given branch or branches with:

mentioning me and:

 cherry-pick to:
 * 1.0.x
 * 2.0.x

SebOpsahl · 2024-05-02T18:05:38Z

The updated test functions were tested with the new implementation (solution proposed for MEN-6877 in another PR) which was successful.

lluiscampos

Good work getting the mock to behave and the tests passing 💪

I think however that we have introduced many unjustified changes in the test flow, and I'd like to discuss or revert them (see comments below).

Your commit is pretty good. Some tips to make it better:

Generally speaking, we are changing the mock and the test flow to accommodate for the changes in the software (your title and description only focuses on the mock)
You put a link to "the other PR" in your commit message 👉 fix: Fix to make mender-connect request a new JWT token on its own, so its not dependent on mender-update to do so mender-connect#134

lluiscampos · 2024-05-03T07:11:41Z

tests/acceptance/mocks/mock_dbus_server.py

-	</interface>
-</node>
-	"""
+    <node>


No objections on changing from tabs to spaces 👍 This was probably what got your editor confused.

lluiscampos · 2024-05-03T07:12:03Z

tests/acceptance/mocks/mock_dbus_server.py

+                    </method>
+            <signal name="JwtTokenStateChange">
+                            <arg type="s" name="token"/>
+                            <arg type="s" name="server_url"/>
+                    </signal>


Nicpick: wrong indentation

Suggested change

</method>

<signal name="JwtTokenStateChange">

<arg type="s" name="token"/>

<arg type="s" name="server_url"/>

</signal>

</method>

<signal name="JwtTokenStateChange">

<arg type="s" name="token"/>

<arg type="s" name="server_url"/>

</signal>

lluiscampos · 2024-05-03T07:14:14Z

tests/acceptance/test_mender-connect.py

@@ -193,14 +202,18 @@ def test_mender_connect_auth_changes(
        """Test that mender-connect can re-establish the connection on D-Bus signals"""

        try:
-            dbus_set_token_and_url(connection, "token1", "http://localhost:5000")


I think this we should keep as it was. The test scenario here is that the server is up and running already when mender-connect starts.

What happened here? I still believe that we should simulate that the server is up and running before mender-connect starts.

Okey, since the tests fail due to timeout when we first set the token and url, and then let mender-connect start. But is successful when we do it after mender-connect has started. I will investigate the issue 👍

tests/acceptance/test_mender-connect.py

lluiscampos · 2024-05-03T07:20:04Z

tests/acceptance/test_mender-connect.py

+            dbus_set_token_and_url(connection, "token1", "http://localhost:5000")
+
+            # sleep for a while to ensure a connection happens
+            time.sleep(60)
+            dbus_emit_signal(connection)
+            signal_time = qemu_system_time(connection)
+
            # wait for first connect
            _ = wait_for_string_in_log(


The sleep here looks suspicious to me. The wait_for_string_in_log function below will wait a given amount of time for the required message, so we shouldn't need to wait before.

You probably meant to wait for mender-connect to start-up before we can instruct the mock to emit the signal, so that will be short sleep between systemctl ... start mender-connect and dbus_emit_signal.

A much better approach that we draft some time ago on the whiteboard would be to have a flag in the mock that can save when mender-connect has called FetchJwtToken and then the test can wait for that to then emit the signal. This is a bit more complex but will be a better test (right now we are blindly assuming that mender-connect requested a token before we emit the signal).

lluiscampos · 2024-05-03T07:20:46Z

tests/acceptance/test_mender-connect.py

+            dbus_set_token_and_url(connection, "token2", "http://localhost:6000")
+            time.sleep(30)
            signal_time = qemu_system_time(connection)
-            dbus_set_token_and_url_and_emit_signal(
-                connection, "token2", "http://localhost:6000"
-            )
+            dbus_emit_signal(connection)


This should stay as it was, why we need to sleep between setting state in the mock and emitting the signal?

lluiscampos · 2024-05-03T07:21:18Z

tests/acceptance/test_mender-connect.py

@@ -277,14 +290,18 @@ def test_mender_connect_reconnect(
        """Test that mender-connect can re-establish the connection on remote errors"""


Much of what I commented in the previous test case is applicable to this one, so I won't review it in detail just yet 👀

lluiscampos · 2024-05-03T07:22:55Z

tests/acceptance/test_mender-connect.py

            _ = wait_for_string_in_log(
-                connection, kill_time, 300, "error reconnecting:",
+                connection, kill_time, 300, "waiting for reconnect",


Why won't in this case eventually come back to error reconnecting? What has changed in mender-connect for this case? 🤔

Based on returns made in the the tests it returned waiting for reconnect given the test case, while the test was made to wait for error reconnecting which made it fail.

…connect This PR is made to change the mocks and testflow in tests to better fit the new implementation of the PR 👉 mendersoftware/mender-connect#134. This PR fixes it so `mender-connect` requests a new JWT token on its own, so its not dependent on `mender-update` to do so. Previously `FetchJWTToken()` wasn’t called which wouldn’t let it call the JWT token independently, which made it dependent on `mender-update`. The proposed fix is using `FetchJWTToken()` to fetch the token, to then for `WaitForJwtTokenStateChange()` to retrieve it in it’s waiting. The old mocks and test flow for the tests seemed to be more tailored to test the functionality with `GetJWTToken()`. This PR proposes a change in the mocks and test flow to better test the new implementation. Changelog: None Ticket: MEN-6877 Signed-off-by: Sebastian Opsahl <[email protected]>

SebOpsahl · 2024-05-26T21:43:16Z

With the new changes made to the actual implementation of the ticket found here 👉 mendersoftware/mender-connect#134 (comment) and the new test implementation where the test ensures an connection instead of just sleeping and assuming an connection everything should look good

SebOpsahl · 2024-05-27T10:05:13Z

@mender-test-bot start pipeline --pr mender-connect/134

mender-test-bot · 2024-05-27T10:05:19Z

Hello 😺 I created a pipeline for you here: Pipeline-1306885344

Build Configuration Matrix

Key	Value
BUILD_BEAGLEBONEBLACK	true
BUILD_CLIENT	true
BUILD_QEMUX86_64_BIOS_GRUB	true
BUILD_QEMUX86_64_BIOS_GRUB_GPT	true
BUILD_QEMUX86_64_UEFI_GRUB	true
BUILD_VEXPRESS_QEMU	true
BUILD_VEXPRESS_QEMU_FLASH	true
BUILD_VEXPRESS_QEMU_UBOOT_UEFI_GRUB	true
META_MENDER_REV	pull/2108/head
RUN_BACKEND_INTEGRATION_TESTS	true
RUN_INTEGRATION_TESTS	true
TEST_QEMUX86_64_BIOS_GRUB	true
TEST_QEMUX86_64_BIOS_GRUB_GPT	true
TEST_QEMUX86_64_UEFI_GRUB	true
TEST_VEXPRESS_QEMU	true
TEST_VEXPRESS_QEMU_FLASH	true
TEST_VEXPRESS_QEMU_UBOOT_UEFI_GRUB	true

lluiscampos · 2024-05-28T07:58:33Z

tests/acceptance/mocks/mock_dbus_server.py

 </node>
-	"""
+    """


It is not a big deal, though. But for the future:

Changing the indentation is unrelated to your changes. The commit would read much cleaner if we could have here only the method that we have added. If you want to modify the indentation it is best to do it on a separate commit.

Oh sorry, the formatting test wouldn't pass unless i changed the formatting. I thought it may have been something with the formatter being updated. Is there any way I can come around the failing test without changing the formatting of the other parts?

Oh sorry, the formatting test wouldn't pass unless i changed the formatting. I thought it may have been something with the formatter being updated. Is there any way I can come around the failing test without changing the formatting of the other parts?

I am not aware of any recent change. We use a fixed version of the formatting tool.

But in any case you could have formatted the existing code first, commit that, then add your changes.

Again, this is not a big deal so don't spend more time on it.

lluiscampos · 2024-05-28T07:59:28Z

tests/acceptance/test_mender-connect.py

@@ -193,14 +202,18 @@ def test_mender_connect_auth_changes(
        """Test that mender-connect can re-establish the connection on D-Bus signals"""

        try:
-            dbus_set_token_and_url(connection, "token1", "http://localhost:5000")


What happened here? I still believe that we should simulate that the server is up and running before mender-connect starts.

lluiscampos · 2024-05-28T08:00:52Z

tests/acceptance/test_mender-connect.py

+                connection, startup_time, 30, "Started Mender Connect service"
+            )
+
+            signal_time = qemu_system_time(connection)


Shouldn't we use signal_time below (line 224) when looking for "Connection established..." in the log?

Oh sorry my bad, fixed it now 👍

lluiscampos · 2024-05-28T08:01:25Z

tests/acceptance/test_mender-connect.py

+                connection, startup_time, 30, "Started Mender Connect service"
+            )
+
+            signal_time = qemu_system_time(connection)


Fixed it now 👍

lluiscampos · 2024-05-28T08:02:13Z

tests/acceptance/test_mender-connect.py

@@ -277,14 +294,22 @@ def test_mender_connect_reconnect(
        """Test that mender-connect can re-establish the connection on remote errors"""

        try:
-            dbus_set_token_and_url(connection, "badtoken", "http://localhost:12345")


Same here, the serer is expected to be running before mender-connect starts

lluiscampos · 2024-05-28T08:02:24Z

tests/acceptance/test_mender-connect.py

            _ = wait_for_string_in_log(
-                connection, kill_time, 300, "error reconnecting:",
+                connection, kill_time, 300, "waiting for reconnect",


lluiscampos · 2024-05-28T08:03:51Z

tests/acceptance/test_mender-connect.py

            # kill the server and wait for error
            with_mock_servers[1].kill()
+            kill_time = qemu_system_time(connection)


This change is probably unnecessary and I vote for reverting it. It is in effect the same thing, but capturing the time before the action (in this case before killing the server) makes it consistent across the test.

Fixed it now :)

SebOpsahl force-pushed the master branch 2 times, most recently from b864e2d to b7a8fb9 Compare May 2, 2024 17:56

SebOpsahl requested a review from lluiscampos May 2, 2024 18:05

lluiscampos requested changes May 3, 2024

View reviewed changes

SebOpsahl force-pushed the master branch 3 times, most recently from 691c712 to 702954b Compare May 26, 2024 21:10

SebOpsahl force-pushed the master branch from 702954b to 5869ea2 Compare May 26, 2024 21:14

SebOpsahl requested review from lluiscampos and a user May 26, 2024 21:43

lluiscampos reviewed May 28, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: updating mocks for tests to test mender-connect JWT request handling #2108

chore: updating mocks for tests to test mender-connect JWT request handling #2108

SebOpsahl commented May 2, 2024 •

edited

Loading

mender-test-bot commented May 2, 2024

SebOpsahl commented May 2, 2024

lluiscampos left a comment

lluiscampos May 3, 2024

lluiscampos May 3, 2024

lluiscampos May 3, 2024

lluiscampos May 28, 2024

SebOpsahl May 29, 2024

lluiscampos May 3, 2024

lluiscampos May 3, 2024

lluiscampos May 3, 2024

lluiscampos May 3, 2024

SebOpsahl May 27, 2024

lluiscampos May 28, 2024

SebOpsahl commented May 26, 2024

SebOpsahl commented May 27, 2024

mender-test-bot commented May 27, 2024

lluiscampos May 28, 2024

SebOpsahl May 29, 2024

lluiscampos May 29, 2024

lluiscampos May 28, 2024

lluiscampos May 28, 2024

SebOpsahl May 29, 2024

lluiscampos May 28, 2024

SebOpsahl May 29, 2024

lluiscampos May 28, 2024

lluiscampos May 28, 2024

lluiscampos May 28, 2024

SebOpsahl May 29, 2024

		@@ -277,14 +290,18 @@ def test_mender_connect_reconnect(
		"""Test that mender-connect can re-establish the connection on remote errors"""

chore: updating mocks for tests to test mender-connect JWT request handling #2108

Are you sure you want to change the base?

chore: updating mocks for tests to test mender-connect JWT request handling #2108

Conversation

SebOpsahl commented May 2, 2024 • edited Loading

mender-test-bot commented May 2, 2024

SebOpsahl commented May 2, 2024

lluiscampos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SebOpsahl commented May 26, 2024

SebOpsahl commented May 27, 2024

mender-test-bot commented May 27, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SebOpsahl commented May 2, 2024 •

edited

Loading