Skip to content

Fix unit test borkage due to loading from /dls_sw#867

Merged
rtuck99 merged 1 commit intomainfrom
fix_to_prevent_dls_sw_being_referenced_in_unit_tests
Oct 25, 2024
Merged

Fix unit test borkage due to loading from /dls_sw#867
rtuck99 merged 1 commit intomainfrom
fix_to_prevent_dls_sw_being_referenced_in_unit_tests

Conversation

@rtuck99
Copy link
Contributor

@rtuck99 rtuck99 commented Oct 25, 2024

Prevents unit tests loading files from places where they shouldn't, this is a port from code that's already in the mx-bluesky unit tests.

Instructions to reviewer on how to test:

  1. Do the unit tests pass
  2. Any attempt to load files from BANNED_PATHS inside a unit test will result in an exception (at least inside those files that are in submodules of where the affected conftest.py module is.

Checks for reviewer

  • Would the PR title make sense to a scientist on a set of release notes
  • If a new device has been added does it follow the standards
  • If changing the API for a pre-existing device, ensure that any beamlines using this device have updated their Bluesky plans accordingly
  • Have the connection tests for the relevant beamline(s) been run via dodal connect ${BEAMLINE}

@rtuck99 rtuck99 marked this pull request as ready for review October 25, 2024 09:05
@codecov
Copy link

codecov bot commented Oct 25, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.54%. Comparing base (fe6ab47) to head (54c9f54).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #867      +/-   ##
==========================================
+ Coverage   95.52%   95.54%   +0.01%     
==========================================
  Files         125      125              
  Lines        5343     5343              
==========================================
+ Hits         5104     5105       +1     
+ Misses        239      238       -1     

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

"i24": mock_paths,
}

BANNED_PATHS = [Path("/dls"), Path("/dls_sw")]
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we just ban absolute paths altogether? /dev/null we can still get with os.devnull, otherwise everything should be included with the module

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't want to exclude /tmp as there are lots of things that write to the tmp directory. As it's a patch on the open builtin I didn't want to be too overreaching in case some dependency of ours tries to do something reasonable.
I think this will catch 99% of things, I don't know what other directories we are likely to be touching ourselves.

@rtuck99 rtuck99 requested a review from DiamondJoseph October 25, 2024 10:21
Copy link
Contributor

@DominicOram DominicOram left a comment

Choose a reason for hiding this comment

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

I'm not really sure why we need this? The unit tests should fail in CI if we're doing this accidentally anyway or are we just trying to be extra cautious in catching things locally?

@rtuck99
Copy link
Contributor Author

rtuck99 commented Oct 25, 2024

I'm not really sure why we need this? The unit tests should fail in CI if we're doing this accidentally anyway or are we just trying to be extra cautious in catching things locally?

In theory, yes CI should catch it, however it can be difficult to debug where things are going wrong if it only happens in CI, especially if you don't get all the exception info. We do already have this in mx-bluesky where I introduced it a while back.

Copy link
Contributor

@DominicOram DominicOram left a comment

Choose a reason for hiding this comment

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

I don't think I personally find it useful to have this locally but I have no strong objection to it if it helps others so all good :)

@rtuck99 rtuck99 force-pushed the fix_to_prevent_dls_sw_being_referenced_in_unit_tests branch from 74a344a to 54c9f54 Compare October 25, 2024 15:41
@rtuck99 rtuck99 merged commit a3a3cf6 into main Oct 25, 2024
@rtuck99 rtuck99 deleted the fix_to_prevent_dls_sw_being_referenced_in_unit_tests branch October 25, 2024 15:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants