-
Notifications
You must be signed in to change notification settings - Fork 119
improve fixtures for mss_dir and eventlet server #2959
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.
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
base: stable
Are you sure you want to change the base?
Conversation
tests/fixtures.py
Outdated
| handle_db_reset() | ||
|
|
||
|
|
||
| @pytest.fixture(scope="session") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this temporary directory be newly created for every test to avoid implicit dependencies?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh, yes, good catch
|
Are you sure the changes in d67169f are necessary? CI passes without them on macOS runners with the usual (un-)reliability. |
tests/fixtures.py
Outdated
| ctx = multiprocessing.get_context("fork") | ||
| process = ctx.Process(target=eventlet.wsgi.server, args=(socket, app), daemon=True) | ||
|
|
||
| def _serve_in_child(conn, _app, _host, _scheme): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Starting parameters with an underscore usually indicates that they are unused AFAIU.
tests/fixtures.py
Outdated
| try: | ||
| _app.config["URL"] = url | ||
| except Exception: | ||
| # If app/config is not writable for some reason, still start server. | ||
| pass |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is bad because now you can't rely on the correctness of this config value. If it is used somewhere then it must be set properly, if it is not used then we should never set it, IMO.
I recheck, I had first tried a PR with the first none optimal change. Lets see. |
|
on my macOS 26.2 system I get currently many "RuntimeError: Server did not start within 5 seconds" 18 passed, 16 skipped, 67 warnings, 358 errors in 84.93s (0:01:24) |
Here we see an attempt to use 8 times 52141 and 4 times 52487 |
|
Another run 567 passed, 16 skipped, 92 warnings, 9 errors in 113.76s (0:01:53) Maybe the problem is that not each test has a different port? here we see 2 times 54919, 5 times 55162 |
|
Port selection is not the issue, the OS selects a free port for us which should be race-free even in the presence of many unrelated processes. The reason why multiple errors show the same port number is because those tests share the same fixture instance for the server. E.g. a mswms server fixture is requested at the class-level here: MSS/tests/_test_msui/test_wms_control.py Lines 52 to 56 in 27a2fb1
|
|
FWIW I know that it passes in CI because I tried it here: https://github.com/matrss/MSS/actions/runs/20426406249/job/58694840047 It just required a bunch of restarts of the macOS 14 tests, which seem to be the most unreliable. But I am not (yet) convinced that the additional changes made it any better. |
On github there is usually not a ~/mss for the account running the tests. But for a user trying the mswms with demodata has some python config files there. Tests should not have the option to change user data or can get different results because of userdata. |
|
I meant the tests pass in CI without the server startup modifications but with the change to the mss_dir configuration, so I don't quite understand your comment... |
Purpose of PR?:
Fixes #2958