Standardize event catching during repository unit tests #10153

QMalcolm · 2024-05-15T23:05:25Z

resolves #9947

Problem

Any modifications to the global event manager during unit tests would affect other tests unless properly cleaned up
There wasn't a good way to check what events were fired during unit tests

Previously if we modified the global event manager during unit tests, we then had to clean up the event manager at the end of the test. No clean up necessary. We also moved the EventCatcher class

Solution

In 67c87fd we added a new fixture always_clean_event_manager to the unit test suite
- because we use autouse=True the fixture is used for every unit test. Thus every unit test will have clean state (so long as we aren't multi-threading our unit tests, which we currently don't)
In bcb2efc we moved the EventCatcher class to tests.utils
- The EventCatcher.catch can be added to the callbacks of the event manager to check that certain event types happen
- Example here

Checklist

I have read the contributing guide and understand what's expected of me
I have run this code in development and it appears to resolve the stated issue
This PR includes tests, or tests are not required/relevant for this PR
This PR has no interface changes (e.g. macros, cli, logs, json artifacts, config files, adapter interface, etc) or this PR has already received feedback and approval from Product or DX
This PR includes type annotations for new and modified functions

Previously we weren't mocking the global event manager in this test. This meant that we also had to clean up the global event manager afterward because changes we made to it would be carried over to other tests. Having to do so is tedious and somewhat annoying, which raised the frustration with writing unit tests that involve the event manager. Although mocking the event manager in this specific test isn't high impact, it is setting up a paradigm that allows us to more easily write unit tests involving the event manager.

…manager We want to be able to modify the global event manager during unit tests. Previously to do so we had to modify the real global event manager and then clean it up at the end of the test. In the previous commit, we updated a test to mock the global event manager, which worked successfully and meant no more cleanup was necessary. This commit takes that work and creates a fixture. Now any test that takes in that fixture as a parameter will have the global event manager be mocked.

…r state

…ging.py`

…ests

codecov · 2024-05-15T23:11:57Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.62%. Comparing base (32a7f82) to head (67c87fd).

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #10153      +/-   ##
==========================================
- Coverage   88.65%   88.62%   -0.03%     
==========================================
  Files         180      180              
  Lines       22422    22422              
==========================================
- Hits        19878    19872       -6     
- Misses       2544     2550       +6

Flag	Coverage Δ
integration	`86.06% <ø> (-0.02%)`	⬇️
unit	`62.99% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

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

QMalcolm · 2024-05-16T00:41:02Z

Odd that the tests are failing here but not locally. The unit test that is failing is a unit test we'd expect to fail if the event manager changes that happen in test_specify_max_bytes (previously test_setup_event_logger_specify_max_bytes) weren't cleaned up via cleanup_event_logger prior to moving to the mocked event logger. Moving to the mocked event logger solved that problem locally, but it doesn't seem to have worked in the github action 🤔

EDIT Correction, it is broken locally. They break starting with e08c448

QMalcolm · 2024-05-16T16:28:16Z

Okay so I've been doing some investigating. The tests start breaking with the change from e08c448. This is odd though because the change in that commit is renaming the test test_setup_event_logger_specify_max_bytes to test_specify_max_bytes and moving it from tests/unit/test_functions.py to tests/unit/events/test_logging.py. You wouldn't think that moving a test would break an unrelated test tests/unit/graph/test_selector.py::test_run_specs[a,b--] nor alter how fixtures are working 🤔 Gonna take a closer look at e08c448, and might possibly revert it.

QMalcolm · 2024-05-16T16:30:40Z

Reverting e08c448 does solve the problem 🤯 I mean this makes sense given that we identified that commit as the problem, but I still don't understand why the commit is problematic.

QMalcolm

Some investigation details

QMalcolm · 2024-05-16T16:38:55Z

tests/unit/events/test_logging.py

@@ -1,7 +1,10 @@
+from argparse import Namespace


Reverting this commit fixes our broken tests/unit/graph/test_selector.py::test_run_specs[a,b--] test problem. I don't understand why.

QMalcolm · 2024-05-16T16:41:41Z

tests/unit/events/test_logging.py

+        set_from_args(args, {})
+        setup_event_logger(get_flags())


One theory I had was that although this commit primarily just moves the test, I also made one small change which is highlighted here. Specifically, in this test previously, these methods were called via the module instead of directly. That is these two lines were

flags.set_from_args(args, {}) setup_event_logger(flags.get_flags())

However changing these lines back to what they were and the the associated import did not solve the broken test issue we're seeing 🤔

ChenyuLInx

Thanks for the detailed PR description!

ChenyuLInx · 2024-05-16T17:17:02Z

For CI breaking I think it might be changing the name caused order of test running changed

QMalcolm · 2024-05-16T17:31:05Z

For CI breaking I think it might be changing the name caused order of test running changed

Good timing! I just reached the same conclusion 😂 test_specify_max_bytes is modifying the global flags, and I think this is carrying over and causing the test failure. Previously this test happened in a different order, meaning one of two things was happening:

There was a test coming after test_specify_max_bytes but before the current breaking test which was resetting the flags previously
The test test_specify_max_bytes was happening after the breaking test previously

Previously we had started _mocking_ the global event manager. We did this because we though that meant anything we did to the global event manager, including modifying it via things like `setup_event_logger`, would be automatically cleaned up at the end of any test using the fixture because the mock would go away. However, this understanding of fixtures and mocking was incorrect, and the global event manager wouldn't be properly isolated/reset. Thus we changed to fixture to instead cleanup the global event manager before any test that uses it and by using `autouse=True` in the fixture definition we made it so that every unit test uses the fixture. Note this will no longer be viabled if we every multi-thread our unit testing as the event manager isn't actually isolated, and thus two tests could both modify the event manager at the same time.

QMalcolm · 2024-05-16T22:39:07Z

Turns out that mocking the global event manager wasn't isolating the event manager by test. Which is unfortunate. We now instead cleanup the event manager before every unit test. Which isn't as nice, but gets the job done.

QMalcolm added 5 commits May 15, 2024 13:55

Add unit test to assert setup_config_logger clears the event manage…

38bcde5

…r state

Move setup_event_logger tests from test_functions.py to `test_log…

e08c448

…ging.py`

Move EventCatcher to tests.utils for use in unit and functional t…

bcb2efc

…ests

QMalcolm added the Skip Changelog Skips GHA to check for changelog file label May 15, 2024

QMalcolm requested a review from a team as a code owner May 15, 2024 23:05

cla-bot bot added the cla:yes label May 15, 2024

QMalcolm commented May 16, 2024

View reviewed changes

ChenyuLInx approved these changes May 16, 2024

View reviewed changes

QMalcolm merged commit 341803d into main May 16, 2024
60 checks passed

QMalcolm deleted the qmalcolm--9947-standard-event-catching-during-repository-unit-tests branch May 16, 2024 22:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Standardize event catching during repository unit tests #10153

Standardize event catching during repository unit tests #10153

QMalcolm commented May 15, 2024 •

edited

Loading

codecov bot commented May 15, 2024 •

edited

Loading

QMalcolm commented May 16, 2024 •

edited

Loading

QMalcolm commented May 16, 2024

QMalcolm commented May 16, 2024 •

edited

Loading

QMalcolm left a comment

QMalcolm May 16, 2024

QMalcolm May 16, 2024 •

edited

Loading

ChenyuLInx left a comment

ChenyuLInx commented May 16, 2024

QMalcolm commented May 16, 2024

QMalcolm commented May 16, 2024

Standardize event catching during repository unit tests #10153

Standardize event catching during repository unit tests #10153

Conversation

QMalcolm commented May 15, 2024 • edited Loading

Problem

Solution

Checklist

codecov bot commented May 15, 2024 • edited Loading

Codecov Report

QMalcolm commented May 16, 2024 • edited Loading

QMalcolm commented May 16, 2024

QMalcolm commented May 16, 2024 • edited Loading

QMalcolm left a comment

Choose a reason for hiding this comment

QMalcolm May 16, 2024

Choose a reason for hiding this comment

QMalcolm May 16, 2024 • edited Loading

Choose a reason for hiding this comment

ChenyuLInx left a comment

Choose a reason for hiding this comment

ChenyuLInx commented May 16, 2024

QMalcolm commented May 16, 2024

QMalcolm commented May 16, 2024

QMalcolm commented May 15, 2024 •

edited

Loading

codecov bot commented May 15, 2024 •

edited

Loading

QMalcolm commented May 16, 2024 •

edited

Loading

QMalcolm commented May 16, 2024 •

edited

Loading

QMalcolm May 16, 2024 •

edited

Loading