8000 Adopt subprocess fixture for testing. by MatthewHambley · Pull Request #391 · MetOffice/fab · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Adopt subprocess fixture for testing. #391

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

Merged
merged 40 commits into from
May 6, 2025

Conversation

MatthewHambley
Copy link
Collaborator

Rather than variations of the same hand-mocking of shelling out to a tool this change takes advantage of an existing fixture.

It also pulls in a fixture to create a fake filing system in memory which should be faster and less prone to faults based on what the developer has in their file tree.

@MatthewHambley MatthewHambley marked this pull request as ready for review March 4, 2025 11:58
@MatthewHambley MatthewHambley requested review from a team and MGEX82 and removed request for a team March 4, 2025 11:58
@yaswant yaswant requested a review from hiker March 4, 2025 14:04
@MatthewHambley MatthewHambley removed the request for review from MGEX82 March 4, 2025 14:17
Copy link
Collaborator
@hiker hiker left a comment

Choose a reason for hiding this comment

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

I only got through parts of it, will try to continue either later today or tomorrow.

Copy link
Collaborator
@hiker hiker left a comment

Choose a reason for hiding this comment

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

8000

Still more to come, sorry

Copy link
Collaborator
@hiker hiker left a comment

Choose a reason for hiding this comment

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

OK, I am stopping here and pass it back to you (extra long weekend) - main reason: could you explain to me the advantage of using fs? I have the feeling I am missing something :) So basically, it is a temporary directory, that is just automatically cleared at the end of a test. In my (PSyclone-heavy ;) ) experience, I often use the files in tmp to debug failing tests. So if files are instead created in tmp, I can't debug them to see e.g. what might be wrong. We also have a fixture for changing into tmpdir, which seems to be the equivalent of fs, except in case of failures you can analyse the files. I would also prefer slightly smaller PRs ;) I am aware that this is already a split of an even larger one, but I am sure I missed a fs fixture a couple of times before wondering what that might be ;) I'll pass it back to you at least for clarification of fs, and pick it up next week.

@hiker
Copy link
Collaborator
hiker commented Mar 7, 2025

Oh, also bringing it up to recent develop would be great - the psyclone changes in master are causing the conflict ;)

@MatthewHambley
Copy link
Collaborator Author

OK, I am stopping here and pass it back to you (extra long weekend) - main reason: could you explain to me the advantage of using fs?

The thing which fs brings to the party over tmp_path is that it is a memory filing system so it is much faster. Unit tests should, after all, be fast.

As a secondary advantage, being a whole filing system, you can use absolute filenames in safety which means you avoid having to add tmp_path to the front of everything. Maybe a small win.

I take your point about debugging and access to the generated files but I think the run-time advantages out way them since tests are run much more often than they are written.

@MatthewHambley
Copy link
Collaborator Author

Note that GitHub C.I. has become a little confused as the Python versions to test with has changed. This leaves old deprecated versions in the list of required runs even though they aren't. I can force commit once you give an approval.

@MatthewHambley MatthewHambley requested a review from hiker April 15, 2025 08:17
@yaswant
Copy link
Collaborator
yaswant commented Apr 15, 2025

Note that GitHub C.I. has become a little confused as the Python versions to test with has changed. This leaves old deprecated versions in the list of required runs even though they aren't. I can force commit once you give an approval.

@MatthewHambley I've removed 3.7 and 3.8 builds from the branch protection rules for develop. Time to include 3.11, 3.12 (or even 3.13.3 if needed) in the GitHub Action workflow #405

Copy link
Collaborator
@hiker hiker left a comment

Choose a reason for hiding this comment

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

Thanks for addressing my comments. Nice update. Proceeding to merge.

@hiker hiker merged commit d6755c5 into MetOffice:develop May 6, 2025
3 checks passed
@hiker
Copy link
Collaborator
hiker commented May 6, 2025

Note that I have added this PR to the list of PRs to go into main (see #353). I think it would be best if we merge this in after the other 6 PRs that are currently waiting for review. It seems not to clash with the profiler profiles (surprisingly :) ). I think it's best if you don't delete the branch (and just updated it from main as the other 6 PRs go in), that should hopefully minimise conflicts.

MatthewHambley added a commit to MatthewHambley/fab that referenced this pull request Jun 16, 2025
Rather than variations of the same hand-mocking of shelling out to a
tool this change takes advantage of an existing fixture
676A
.

It also pulls in a fixture to create a fake filing system in memory
which should be faster and less prone to faults based on what the
developer has in their file tree.
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
0