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

Jump to bottom

Properly separate unit tests from system tests WRT subprocess #735

Merged
merged 7 commits into from
Dec 16, 2024
Merged

Properly separate unit tests from system tests WRT subprocess #735

merged 7 commits into from
Dec 16, 2024

Conversation

callumforrester
Copy link
Contributor

Fixes #601

@codecov Codecov
Copy link

codecov bot commented Nov 28, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.92%. Comparing base (2d77d61) to head (2d118b8).

Additional details and impacted files 
@@            Coverage Diff             @@
##             main     #735      +/-   ##
==========================================
- Coverage   93.19%   92.92%   -0.28%     
==========================================
  Files          37       37              
  Lines        2072     2063       -9     
==========================================
- Hits         1931     1917      -14     
- Misses        141      146       +5

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@callumforrester callumforrester marked this pull request as ready for review December 2, 2024 10:00
@callumforrester
Copy link
Contributor Author

Tagged @joeshannon for interest

@callumforrester
Copy link
Contributor Author

See #601 for the old 20 slowest tests

================================ slowest 20 durations =================================
0.20s setup    tests/unit_tests/worker/test_task_worker.py::test_full_queue_raises_WorkerBusyError
0.13s setup    tests/unit_tests/worker/test_task_worker.py::test_plan_failure_recorded_in_active_task
0.12s setup    tests/unit_tests/worker/test_task_worker.py::test_clear_task_span_ok
0.12s setup    tests/unit_tests/worker/test_task_worker.py::test_begin_task_span_ok
0.11s setup    tests/unit_tests/worker/test_task_worker.py::test_submit_task_span_ok
0.11s call     tests/unit_tests/core/test_context.py::test_add_plan_from_module
0.11s call     tests/unit_tests/service/test_openapi.py::test_schema_updated
0.11s setup    tests/unit_tests/worker/test_task_worker.py::test_get_tasks_by_status[PENDING-expected_task_ids1]
0.11s setup    tests/unit_tests/worker/test_task_worker.py::test_does_not_allow_simultaneous_running_tasks
0.11s setup    tests/unit_tests/worker/test_task_worker.py::test_submit_before_start_pending
0.11s call     tests/unit_tests/test_cli.py::test_env_timeout
0.10s setup    tests/unit_tests/worker/test_task_worker.py::test_stop_span_ok
0.10s setup    tests/unit_tests/worker/test_task_worker.py::test_restart_leaves_task_pending
0.10s setup    tests/unit_tests/worker/test_task_worker.py::test_begin_task_blocks_until_current_task_set
0.10s setup    tests/unit_tests/worker/test_task_worker.py::test_clear_nonexistent_task
0.10s setup    tests/unit_tests/worker/test_task_worker.py::test_produces_worker_events[1]
0.10s setup    tests/unit_tests/worker/test_task_worker.py::test_clear_task
0.10s call     tests/unit_tests/service/test_interface.py::test_stomp_config
0.10s setup    tests/unit_tests/worker/test_task_worker.py::test_get_tasks_by_status[COMPLETE-expected_task_ids2]
0.10s setup    tests/unit_tests/worker/test_task_worker.py::test_worker_and_data_events_produce_in_order

DiamondJoseph
DiamondJoseph requested changes Dec 2, 2024
View reviewed changes
src/blueapi/service/runner.py Outdated
) -> T:
"""Call the supplied function, passing the current Span ID, if one
exists,from the observability context inro the _rpc caller function.
exists,from the observability context inro the import_and_run_function
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
exists,from the observability context inro the import_and_run_function
exists,from the observability context into the import_and_run_function

nit

src/blueapi/service/runner.py Outdated
add_span_attributes(
{"_use_subprocess": self._use_subprocess, "_config": str(self._config)}
)
add_span_attributes({"_use_subprocess": True, "_config": str(self._config)})
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
add_span_attributes({"_use_subprocess": True, "_config": str(self._config)})
add_span_attributes({"_config": str(self._config)})

Copy link
Contributor

Choose a reason for hiding this comment

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

see below

src/blueapi/service/runner.py Outdated
When this is deserialized in and run by the subprocess, this will allow
its functions to use the corresponding span as their parent span."""

add_span_attributes({"use_subprocess": True})
Copy link
Contributor

Choose a reason for hiding this comment

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

Must

Suggested change
add_span_attributes({"use_subprocess": True})

As discussed offline: we are always using the same value for use_subprocess, as it is no longer an instance variable. Either find a way to mark what type of subprocess_factory was used or remove.

@callumforrester
Mock subprocess in runner tests
2183db0 
- Mock subprocess in runner tests
- Remove ability of runner to invoke function in the same process, this was previously only used in tests
- Expose _rpc function as import_and_run_function, write separate tests for it as it was no longer behind the mock
@callumforrester
Convert REST API tests into true unit tests
e27cb5e 
Mock runner, test REST calls only
@callumforrester
Test that runner creates subprocess
512e52b
@callumforrester
WIP: Add system test for environment reload failure
2f594ca
@callumforrester
Revert system test changes
6820b57
@callumforrester
Remove extra span attrs and fix typo
7be88ea
@callumforrester
Copy link
Contributor Author

Apologies, I mistakenly believed this was merged. I have rebased and updated the new tests to reflect this scheme. Unfortunately a new slow test has been introduced in the auth code since I did this PR. I suggest making that into a new issue since it only adds 2s to the test time and the acceptance criteria for #601 are still met. Current test times below:

======================================================================= slowest 20 durations ========================================================================
2.01s call     tests/unit_tests/service/test_authentication.py::test_poll_for_token_timeout
0.24s setup    tests/unit_tests/worker/test_task_worker.py::test_restart_leaves_task_pending
0.20s setup    tests/unit_tests/worker/test_task_worker.py::test_multi_stop
0.19s call     tests/unit_tests/service/test_openapi.py::test_schema_updated
0.18s setup    tests/unit_tests/worker/test_task_worker.py::test_stop_doesnt_hang
0.17s setup    tests/unit_tests/worker/test_task_worker.py::test_stop_is_idempontent_if_worker_not_started
0.16s setup    tests/unit_tests/worker/test_task_worker.py::test_submit_task
0.16s setup    tests/unit_tests/worker/test_task_worker.py::test_multi_start
0.16s teardown tests/unit_tests/service/test_interface.py::test_begin_task_no_task_id
0.15s setup    tests/unit_tests/worker/test_task_worker.py::test_restart
0.14s teardown tests/unit_tests/service/test_interface.py::test_get_device
0.14s call     tests/unit_tests/service/test_interface.py::test_begin_task
0.14s setup    tests/unit_tests/worker/test_task_worker.py::test_submit_multiple_tasks
0.13s call     tests/unit_tests/service/test_interface.py::test_get_tasks_by_status
0.13s setup    tests/unit_tests/worker/test_task_worker.py::test_worker_and_data_events_produce_in_order
0.12s setup    tests/unit_tests/worker/test_task_worker.py::test_stop_with_task_pending
0.12s call     tests/unit_tests/service/test_interface.py::test_stomp_config
0.12s teardown tests/unit_tests/service/test_interface.py::test_get_oidc_config
0.12s setup    tests/unit_tests/worker/test_task_worker.py::test_produces_worker_events[2]
0.12s setup    tests/unit_tests/worker/test_task_worker.py::test_full_queue_raises_WorkerBusyError

@callumforrester callumforrester mentioned this pull request Dec 16, 2024
Closed
@callumforrester callumforrester merged commit 13c6a59 into main Dec 16, 2024
29 checks passed
@callumforrester callumforrester deleted the 601-faster-runner-tests branch December 16, 2024 11:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@DiamondJoseph DiamondJoseph DiamondJoseph approved these changes

@joeshannon joeshannon Awaiting requested review from joeshannon

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Tests take too long
2 participants
@callumforrester @DiamondJoseph