Refactor tests and setup #1968

ReimarBauer · 2023-08-30T11:04:36Z

Purpose of PR?:

Fixes #1957 for the mscolab part

Does this PR introduce a breaking change?

If the changes in this PR are manually verified, list down the scenarios covered::

Additional information for reviewer? :
the PR reduces the amount of servers needed for mscolab tests. Also a bug in the decription for a category change fixed.
For develop the tests were not adjusted to the UI refactoring. The conftest has removed all QMessage boxes.
Some errors we had were related that we don't loaded an initial flight track to the window for tests. Other Time out Blockers occure when some of the Questions are not mocked.

Does this PR results in some Documentation changes?
If yes, include the list of Documentation changes

Checklist:

Bug fix. Fixes #
New feature (Non-API breaking changes that adds functionality)
PR Title follows the convention of <type>: <subject>
Commit has unit tests

ReimarBauer · 2023-08-31T04:24:13Z

this should be synced with work of @matrss #1967

ReimarBauer · 2023-08-31T04:27:17Z

conftest.py

@@ -210,32 +210,6 @@ def _load_module(module_name, path):
 _load_module("mscolab_settings", path)




Removing this will Error all tests with an unhandled QMessageBox

I don't think that is true. I just had a test pass with this fixture removed even though it showed a message box and failed with my modified fixture.

ReimarBauer · 2023-08-31T04:28:17Z

conftest.py

@@ -248,3 +222,7 @@ def configure_testsetup(request):
        VIRT_DISPLAY.stop()
    else:
        yield
+


the new approach is not to start on any test a mscolab service but once per test class. We need at the end to terminate all processes

Is that really a good idea? We could run into issues where one test affects the outcome of another due to the shared mscolab instance, which could be a nightmare to debug. Also, I do not really like the implementation with this global variable here, couldn't this be a fixture sort of like this (pseudocode):

@pytest.fixture() def mscolab_server(): server = start_mscolab_server() yield server server.shutdown()

?

Every test that needs a running mscolab server then should just use the fixture. That way we could even use the scope of the fixture to decide how "shared" we want the instance to be.

For what it's worth, I would prefer to get rid of test classes entirely, anyway. A yield fixture is much better suited for setup and teardown code, which leaves classes only as an organizational unit to group tests together. But modules, in turn, do that better.

it is not a global variable, it is defined as module local variable.
That's a great article about https://betterprogramming.pub/alternatives-to-using-globals-in-python-a3b2a7d5411b

we need for xdist calls servers running on different ports,

try it.

what do you mean by removing test classes entirely, I think setup and teardown may become complicated. But lets see.

each test cleans up the database, and xdist is currently called on --dist loadfile
The requirement by this is only different port per file.

I also like to have different servers on each run, which I introduced before.

Anyway any test needs independent from each other. So not having a test which creates data and another test which tries to vaildate something based on that.

what do you mean by removing test classes entirely, I think setup and teardown may become complicated. But lets see.

The recommended way (by pytest) to have setup and teardown code is using a yield fixture. You can read more about that here: https://docs.pytest.org/en/6.2.x/fixture.html#yield-fixtures-recommended
They have an example there to demonstrate how user creation and deletion would work with fixtures, in an example testing a hypothetical email server. No class needed.

Anyway any test needs independent from each other. So not having a test which creates data and another test which tries to vaildate something based on that.

Hypothetically, what would happen with these shared mscolab servers if we had a test for user deletion trying to delete user "UV10@uv10" in Test_Mscolab_connect_window, but also try to login to this user in test_login? My concern is that we might run into similar, but less obvious, race conditions in the future.

it is not a global variable, it is defined as module local variable.

In

MSS/conftest.py

Lines 226 to 228 in e60909c

def pytest_sessionfinish(session, exitstatus):

for process in MSCOLAB_PROCESSES:

process.terminate()

we terminate all processes once when we are finished with running the test suite. Isn't MSCOLAB_PROCESSES a global variable in the scope of the test session, then? We have one instance of this variable in which we accumulate all the running mscolab processes while executing the tests.

If I understand that right, with this PR we would also keep all the processes running until the end of the session, instead of stopping them as soon as they are not needed anymore.

what do you mean by removing test classes entirely, I think setup and teardown may become complicated. But lets see.

The recommended way (by pytest) to have setup and teardown code is using a yield fixture. You can read more about that here: https://docs.pytest.org/en/6.2.x/fixture.html#yield-fixtures-recommended They have an example there to demonstrate how user creation and deletion would work with fixtures, in an example testing a hypothetical email server. No class needed.

Anyway any test needs independent from each other. So not having a test which creates data and another test which tries to vaildate something based on that.

Hypothetically, what would happen with these shared mscolab servers if we had a test for user deletion trying to delete user "UV10@uv10" in Test_Mscolab_connect_window, but also try to login to this user in test_login? My concern is that we might run into similar, but less obvious, race conditions in the future.

connect_window and
test_login has to setup the user,
after each test per teardown the database is cleared.
Any test has to do all the steps for testing an approach.

when the tests are paralell running by xdist, any worker creates new tmp dirs
two workers don't access the same database

We had the described problems before, that's why tests currently not based on some others.

I would prefer more functional tests, without using the UI. At least everything what can be tested without the UI should be done.

At least everything what can be tested without the UI should be done.

I have to disagree on that. We should test at the "level" at which users interact with the application as well, and that is the UI. There is no point in testing if a login works if we don't test that the actual login button works also.

Or to phrase it differently: everything that can be done with the UI cannot be tested without it.

we do UI tests already in many ways also by the tutorials, But we also have a lot helper functions which also work without the UI and these should become tested if possibile without the UI.

ReimarBauer · 2023-08-31T04:38:13Z

.github/workflows/testing.yml

@@ -96,7 +96,7 @@ jobs:
        && source /opt/conda/etc/profile.d/conda.sh \
        && source /opt/conda/etc/profile.d/mamba.sh \
        && mamba activate mss-${{ inputs.branch_name }}-env \
-        && pytest -vv -n 6 --dist loadfile --max-worker-restart 0 tests \


currently will loose all tests assigned to a worker when that crashes

ReimarBauer · 2023-08-31T04:40:09Z

mslib/msui/mscolab.py

@@ -1056,8 +1056,11 @@ def handle_delete_operation(self):
                        self.reload_operations()
                        self.signal_operation_removed.emit(self.active_op_id)
                        logging.debug("activate local")
-                        self.ui.listFlightTracks.setCurrentRow(0)
-                        self.ui.activate_selected_flight_track()
+                        if self.ui.listFlightTracks.count() == 0:


in mscolab we can remove "all" operations from the list, while we can't in the local flight path (left site of the UI)
When we do this, we have to switch to an active operation, which is done by switch_to_local

ReimarBauer · 2023-08-31T04:41:04Z

mslib/msui/mscolab.py

@@ -1986,11 +1989,9 @@ def logout(self):

        self.operation_archive_browser.hide()

-        # Don't try to activate local flighttrack while testing
-        if "pytest" not in sys.modules:


this is not needed when the tests and code are updated

ReimarBauer · 2023-08-31T04:51:19Z

504 passed, 27 skipped, 208 warnings in 217.48s (0:03:37)

matrss · 2023-08-31T07:42:49Z

tests/_test_msui/test_mscolab.py

-    def test_login(self):
+    @mock.patch("PyQt5.QtWidgets.QMessageBox.question", return_value=QtWidgets.QMessageBox.No)
+    def test_login(self, mockquestion):
        self._connect_to_mscolab()
        self._login(self.userdata[0], self.userdata[2])


I disagree with the mocking here, we shouldn't just "click no" and be done with it. The issue here, why the message box pops up at all, is that in the msui config used in the test the "MSS_auth" does not have self.userdata[0] set as the username/email for the mscolab server we connect to here. With my approach to this:

MSS/tests/_test_msui/test_mscolab.py

Lines 123 to 126 in ee1c48c

def test_login(self):

self._connect_to_mscolab()

modify_config_file({"MSS_auth": {self.url: self.userdata[0]}})

self._login(self.userdata[0], self.userdata[2])

the message box disappears entirely. We could also test that the message box pops up as expected when there is this mismatch in the config, but then we would have to also add a mockquestion.assert_called_once_with(<args>) in every test. I think that is something we should just do once in a separate test case.

Case in point: there was a message box appearing in test_get_airspaces saying there was no airspace data in the file. That made no sense. It led me to discover this bug: https://github.com/Open-MSS/MSS/pull/1967/files#diff-ce023988bd24cea5863ed80b6ef4058629448da2f64480cd8dc31f06c5186a57R254-R256

On the other hand, in test_get_airspaces_missing_data we expected exactly this message box, so there we should mock the message box but then also assert that we get what we expect: https://github.com/Open-MSS/MSS/pull/1967/files#diff-d76a934985b02e5c8cafb478f5e5221eacd80d770c69798e9370ebb2232d8f9dR206-R215

Obviously this argument applies to most of the other locations where this message box is mocked here as well.

that's why I thought when you incorporate changes of this into your branch you can decide this on the merge or cherry pick, afterwards we close this PR and you can verify the other ideas.

I don't think we should combine this larger architectural change with my, I'd argue, "less invasive" PR. I almost have my PR in a state in which I think it would be merge-able, getting rid of these warnings now and cleaning up the CI outputs, and then we could have a look at this more fundamental PR.

ReimarBauer · 2023-10-08T10:17:09Z

won't be done

ReimarBauer added 30 commits August 30, 2023 10:52

prepared for py311

1f2185c

skipped needs review

ff271a0

enables worker restart

8f1dd20

worker restart

582172d

skip test

2e849b5

disabled fixture

23119c6

disabled test without asserts

f5e4d39

skip test

8d3ee45

module added

095aad0

test skipped

beceb89

test skipped

1062b18

test skipped

3202c58

skip tests

57794b4

trying other ports

28f79b8

flake8

73aea5d

skip test

27ab824

work started

7f97018

without py311 for now

dbb0743

undo port range

090e98d

tests need an initial flight path

35a0eba

fixed category change message

118fdef

closing open windows

5596b72

renamed attributes

086f7ca

updated order of tests

e370016

one server per class

ac77cba

flake8

b4d91b6

improved mscolab tests

cc65536

flake8

5f31115

more to mock

9b7f110

changed pytest skip 311 marker

e60909c

ReimarBauer requested review from matrss and joernu76 August 30, 2023 19:23

ReimarBauer commented Aug 31, 2023

View reviewed changes

matrss reviewed Aug 31, 2023

View reviewed changes

ReimarBauer marked this pull request as draft August 31, 2023 13:11

ReimarBauer added 2 commits September 6, 2023 11:05

Merge branch 'develop' into refactor_tests_and_setup

affaab0

flake8

92992be

ReimarBauer closed this Oct 8, 2023

ReimarBauer deleted the refactor_tests_and_setup branch May 25, 2024 07:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor tests and setup #1968

Refactor tests and setup #1968

ReimarBauer commented Aug 30, 2023 •

edited

Loading

ReimarBauer commented Aug 31, 2023

ReimarBauer Aug 31, 2023 •

edited

Loading

matrss Aug 31, 2023 •

edited

Loading

ReimarBauer Aug 31, 2023

matrss Aug 31, 2023 •

edited

Loading

ReimarBauer Aug 31, 2023

ReimarBauer Aug 31, 2023

matrss Aug 31, 2023

matrss Aug 31, 2023

ReimarBauer Aug 31, 2023 •

edited

Loading

ReimarBauer Aug 31, 2023

matrss Aug 31, 2023

ReimarBauer Aug 31, 2023 •

edited

Loading

ReimarBauer Aug 31, 2023

ReimarBauer Aug 31, 2023

ReimarBauer Aug 31, 2023

ReimarBauer commented Aug 31, 2023

matrss Aug 31, 2023 •

edited

Loading

matrss Aug 31, 2023

matrss Aug 31, 2023

ReimarBauer Aug 31, 2023 •

edited

Loading

matrss Aug 31, 2023

ReimarBauer Aug 31, 2023

ReimarBauer commented Oct 8, 2023

		@@ -210,32 +210,6 @@ def _load_module(module_name, path):
		_load_module("mscolab_settings", path)

	def pytest_sessionfinish(session, exitstatus):
	for process in MSCOLAB_PROCESSES:
	process.terminate()

	def test_login(self):
	self._connect_to_mscolab()
	modify_config_file({"MSS_auth": {self.url: self.userdata[0]}})
	self._login(self.userdata[0], self.userdata[2])

Refactor tests and setup #1968

Refactor tests and setup #1968

Conversation

ReimarBauer commented Aug 30, 2023 • edited Loading

ReimarBauer commented Aug 31, 2023

ReimarBauer Aug 31, 2023 • edited Loading

Choose a reason for hiding this comment

matrss Aug 31, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matrss Aug 31, 2023 • edited Loading

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

ReimarBauer Aug 31, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ReimarBauer Aug 31, 2023 • edited Loading

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

ReimarBauer commented Aug 31, 2023

matrss Aug 31, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ReimarBauer Aug 31, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ReimarBauer commented Oct 8, 2023

ReimarBauer commented Aug 30, 2023 •

edited

Loading

ReimarBauer Aug 31, 2023 •

edited

Loading

matrss Aug 31, 2023 •

edited

Loading

matrss Aug 31, 2023 •

edited

Loading

ReimarBauer Aug 31, 2023 •

edited

Loading

ReimarBauer Aug 31, 2023 •

edited

Loading

matrss Aug 31, 2023 •

edited

Loading

ReimarBauer Aug 31, 2023 •

edited

Loading