Skip to content
This repository has been archived by the owner on Sep 2, 2024. It is now read-only.

(#1252) Fix logging unit test failure #1288

Merged
merged 2 commits into from
Apr 3, 2024

Conversation

rtuck99
Copy link
Contributor

@rtuck99 rtuck99 commented Apr 2, 2024

Fixes unit test failures introduced in previous fix for #1252

Link to dodal PR (if required): #N/A

To test:

  1. CI tests pass

@rtuck99 rtuck99 requested a review from d-perl April 2, 2024 13:37
Copy link
Contributor

@d-perl d-perl left a comment

Choose a reason for hiding this comment

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

Looks great, just one thing

if "dodal.beamlines.beamline_utils" in sys.modules:
sys.modules["dodal.beamlines.beamline_utils"].clear_devices()
markers = [m.name for m in item.own_markers]
if item.config.getoption("logging") and "skip_log_setup" in markers:
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this should check for the option - we want to tear down after these tests regardless of whether we are in CI or local, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm yes on reflection I think you are right, the tests will execute even if logging is disabled and may still do stuff.

Copy link
Contributor

@d-perl d-perl left a comment

Choose a reason for hiding this comment

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

great, thanks!

@d-perl d-perl merged commit 3d5ab75 into main Apr 3, 2024
5 checks passed
@d-perl d-perl deleted the 1252_fix_logging_unit_test_failures branch April 3, 2024 11:30
olliesilvester pushed a commit to olliesilvester/mx-bluesky that referenced this pull request Aug 23, 2024
…Source/1252_fix_logging_unit_test_failures

(DiamondLightSource/hyperion#1252) Fix logging unit test failure
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants