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

Refactor tests and setup #1968

Closed
wants to merge 32 commits into from
Closed

Refactor tests and setup #1968

Show file tree
Hide file tree
Changes from 30 commits
Commits
Show all changes
32 commits
Select commit Hold shift + click to select a range
1f2185c
prepared for py311
ReimarBauer Aug 23, 2023
ff271a0
skipped needs review
ReimarBauer Aug 23, 2023
8f1dd20
enables worker restart
ReimarBauer Aug 23, 2023
582172d
worker restart
ReimarBauer Aug 23, 2023
2e849b5
skip test
ReimarBauer Aug 23, 2023
23119c6
disabled fixture
ReimarBauer Aug 24, 2023
f5e4d39
disabled test without asserts
ReimarBauer Aug 24, 2023
8d3ee45
skip test
ReimarBauer Aug 24, 2023
095aad0
module added
ReimarBauer Aug 24, 2023
beceb89
test skipped
ReimarBauer Aug 24, 2023
1062b18
test skipped
ReimarBauer Aug 24, 2023
3202c58
test skipped
ReimarBauer Aug 24, 2023
57794b4
skip tests
ReimarBauer Aug 25, 2023
28f79b8
trying other ports
ReimarBauer Aug 25, 2023
73aea5d
flake8
ReimarBauer Aug 25, 2023
27ab824
skip test
ReimarBauer Aug 27, 2023
7f97018
work started
ReimarBauer Aug 30, 2023
dbb0743
without py311 for now
ReimarBauer Aug 30, 2023
090e98d
undo port range
ReimarBauer Aug 30, 2023
35a0eba
tests need an initial flight path
ReimarBauer Aug 30, 2023
118fdef
fixed category change message
ReimarBauer Aug 30, 2023
5596b72
closing open windows
ReimarBauer Aug 30, 2023
086f7ca
renamed attributes
ReimarBauer Aug 30, 2023
e370016
updated order of tests
ReimarBauer Aug 30, 2023
ac77cba
one server per class
ReimarBauer Aug 30, 2023
b4d91b6
flake8
ReimarBauer Aug 30, 2023
cc65536
improved mscolab tests
ReimarBauer Aug 30, 2023
5f31115
flake8
ReimarBauer Aug 30, 2023
9b7f110
more to mock
ReimarBauer Aug 30, 2023
e60909c
changed pytest skip 311 marker
ReimarBauer Aug 30, 2023
affaab0
Merge branch 'develop' into refactor_tests_and_setup
ReimarBauer Sep 6, 2023
92992be
flake8
ReimarBauer Sep 6, 2023
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
2 changes: 1 addition & 1 deletion .github/workflows/testing.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 \
Copy link
Member Author

Choose a reason for hiding this comment

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

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

&& pytest -vv -n 6 --dist loadfile --max-worker-restart 4 tests \
|| (for i in {1..5} \
; do pytest -vv -n 6 --dist loadfile --max-worker-restart 0 tests --last-failed --lfnf=none \
&& break \
Expand Down
32 changes: 5 additions & 27 deletions conftest.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@
sample_path = os.path.join(os.path.dirname(__file__), "tests", "data")
shutil.copy(os.path.join(sample_path, "example.ftml"), constants.ROOT_DIR)


MSCOLAB_PROCESSES = []
class TestKeyring(keyring.backend.KeyringBackend):
"""A test keyring which always outputs the same password
from Runtime Configuration
Expand Down Expand Up @@ -210,32 +210,6 @@ def _load_module(module_name, path):
_load_module("mscolab_settings", path)


Copy link
Member Author

@ReimarBauer ReimarBauer Aug 31, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Removing this will Error all tests with an unhandled QMessageBox

Copy link
Collaborator

@matrss matrss Aug 31, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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.

@pytest.fixture(autouse=True)
def close_open_windows():
"""
Closes all windows after every test
"""
# Mock every MessageBox widget in the test suite to avoid unwanted freezes on unhandled error popups etc.
with mock.patch("PyQt5.QtWidgets.QMessageBox.question") as q, \
mock.patch("PyQt5.QtWidgets.QMessageBox.information") as i, \
mock.patch("PyQt5.QtWidgets.QMessageBox.critical") as c, \
mock.patch("PyQt5.QtWidgets.QMessageBox.warning") as w:
yield
if any(box.call_count > 0 for box in [q, i, c, w]):
summary = "\n".join([f"PyQt5.QtWidgets.QMessageBox.{box()._extract_mock_name()}: {box.mock_calls[:-1]}"
for box in [q, i, c, w] if box.call_count > 0])
warnings.warn(f"An unhandled message box popped up during your test!\n{summary}")


# Try to close all remaining widgets after each test
for qobject in set(QtWidgets.QApplication.topLevelWindows() + QtWidgets.QApplication.topLevelWidgets()):
try:
qobject.destroy()
# Some objects deny permission, pass in that case
except RuntimeError:
pass


@pytest.fixture(scope="session", autouse=True)
def configure_testsetup(request):
if Display is not None:
Expand All @@ -248,3 +222,7 @@ def configure_testsetup(request):
VIRT_DISPLAY.stop()
else:
yield

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

@matrss matrss Aug 31, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

@ReimarBauer ReimarBauer Aug 31, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

@ReimarBauer ReimarBauer Aug 31, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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.

def pytest_sessionfinish(session, exitstatus):
for process in MSCOLAB_PROCESSES:
process.terminate()
17 changes: 9 additions & 8 deletions mslib/msui/mscolab.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Copy link
Member Author

Choose a reason for hiding this comment

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

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

self.switch_to_local()
else:
self.ui.listFlightTracks.setCurrentRow(0)
self.ui.activate_selected_flight_track()
else:
show_popup(self.ui, "Error", "Entered operation name did not match!")
else:
Expand Down Expand Up @@ -1131,7 +1134,7 @@ def change_category_handler(self):
self.active_operation_category = entered_operation_category
self.reload_operation_list()
self.error_dialog = QtWidgets.QErrorMessage()
self.error_dialog.showMessage("Description is updated successfully.")
self.error_dialog.showMessage("Category is updated successfully.")
else:
show_popup(self.ui, "Error", "Your Connection is expired. New Login required!")
self.logout()
Expand Down Expand Up @@ -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:
Copy link
Member Author

Choose a reason for hiding this comment

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

this is not needed when the tests and code are updated

# activate first local flighttrack after logging out
self.ui.listFlightTracks.setCurrentRow(0)
self.ui.activate_selected_flight_track()
# activate first local flighttrack after logging out
self.ui.listFlightTracks.setCurrentRow(0)
self.ui.activate_selected_flight_track()


class MscolabMergeWaypointsDialog(QtWidgets.QDialog, merge_wp_ui.Ui_MergeWaypointsDialog):
Expand Down
Loading