Skip to content

Always pass feedstock dir Path to migrator tests#3774

Merged
beckermr merged 17 commits intoregro:mainfrom
mgorny:test-tmpdir-cleanup
Feb 27, 2025
Merged

Always pass feedstock dir Path to migrator tests#3774
beckermr merged 17 commits intoregro:mainfrom
mgorny:test-tmpdir-cleanup

Conversation

@mgorny
Copy link
Copy Markdown
Contributor

@mgorny mgorny commented Feb 27, 2025

(work-in-progress posted to facilitate early feedback)

Description:

Update the run_test_migration() function to always expect the top feedstock directory, passed as Path, for both schema versions. Previously, the function would expect feedstock directory for v1 recipes, and the recipe subdirectory for v0 recipes. This improves consistency, as well as modernizes the code.

While at it, rewrite the relevant tests to use the modern tmp_path fixture rather than the deprecated tmpdir fixture.

Checklist:

  • Pydantic model updated or no update needed

Cross-refs, links to issues, etc:

Discussed in #3765.

mgorny and others added 9 commits February 26, 2025 18:53
Enable `CondaForgeYAMLCleanup` migrator for v1 recipes.  Since it works
on `conda-forge.yml` only, it does not require any changes.  So the most
of the changes are related to extending the test.
Fix an exception when the tested v1 recipe has no `license_file` key:

E       TypeError: 'NoneType' object is not subscriptable
Update the `run_test_migration()` function to always expect the top
feedstock directory, passed as `Path`, for both schema versions.
Previously, the function would expect feedstock directory for v1
recipes, and the recipe subdirectory for v0 recipes.  This improves
consistency, as well as modernizes the code.

While at it, rewrite the relevant tests to use the modern `tmp_path`
fixture rather than the deprecated `tmpdir` fixture.
@mgorny mgorny force-pushed the test-tmpdir-cleanup branch from 8128716 to 61f9878 Compare February 27, 2025 14:45
@mgorny
Copy link
Copy Markdown
Contributor Author

mgorny commented Feb 27, 2025

I've rebased it on #3765 to avoid conflicts when it's merged. It's mostly done, still need to update the API for minimigrator and YAML migrator tests.

@codecov
Copy link
Copy Markdown

codecov bot commented Feb 27, 2025

Codecov Report

Attention: Patch coverage is 92.80000% with 9 lines in your changes missing coverage. Please review.

Project coverage is 75.80%. Comparing base (23e982e) to head (1308e90).
Report is 18 commits behind head on main.

Files with missing lines Patch % Lines
tests/test_migrators_v1.py 61.90% 8 Missing ⚠️
tests/test_migration_yaml_migration.py 92.30% 1 Missing ⚠️

❌ Your project status has failed because the head coverage (93.98%) is below the target coverage (95.00%). You can increase the head coverage or adjust the target coverage.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3774      +/-   ##
==========================================
- Coverage   77.30%   75.80%   -1.50%     
==========================================
  Files         134      134              
  Lines       14855    14849       -6     
==========================================
- Hits        11484    11257     -227     
- Misses       3371     3592     +221     

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

@mgorny
Copy link
Copy Markdown
Contributor Author

mgorny commented Feb 27, 2025

Okay, so the changes here include:

  1. Converting migrator tests to use tmp_path instead of tmpdir fixture.
  2. Updating v0 migrator logic to treat tmp_path as the top feedstock directory, rather than the recipe subdirectory.
  3. Removing some redundant logic.

When updating code for tmp_path being a Path, I've followed the following principles:

  1. I've replaced all os.path uses with the appropriate Path methods. However, I've left open() uses in place — I can update these later, possibly replacing them with read_text(), write_text(), etc. if you wish.
  2. I've used / to join paths whenever that did not introduce parentheses; if it would, I used .joinpath() instead (i.e. tmp_path.joinpath("foo").mkdir() rather than (tmp_path / "foo").mkdir().
  3. When combining multiple directory levels, I've used ... / "foo/bar" rather than ... / "foo" / "bar".

I can change that if you prefer.

I can also address the remaining uses of tmpdir in tests, but I'd prefer doing that separately to keep the diff from becoming too large.

@beckermr
Copy link
Copy Markdown
Contributor

I think I got the conflicts here. sorry for the squash!

@mgorny mgorny marked this pull request as ready for review February 27, 2025 17:00
@mgorny
Copy link
Copy Markdown
Contributor Author

mgorny commented Feb 27, 2025

I think I got the conflicts here. sorry for the squash!

No problem, I treat the followup commits as dispensable.

Copy link
Copy Markdown
Contributor

@beckermr beckermr left a comment

Choose a reason for hiding this comment

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

I have two questions.

  1. When do we need to call str() on Path objects? It is not always done in some spots versus others (e.g., when passing a path to the migrator methods) and I do not understand why.
  2. The use of / in strings for joining paths like this pathobj / "foo/bar" appears like it would break using the test suite on windows. Is that true? If so, do we care?

@mgorny
Copy link
Copy Markdown
Contributor Author

mgorny commented Feb 27, 2025

I have two questions.

1. When do we need to call `str()` on `Path` objects? It is not always done in some spots versus others (e.g., when passing a path to the migrator methods) and I do not understand why.

I've tried to put it to cf-scripts' functions where str was passed previously, to avoid type mismatches. However, I've left Path objects where they work in stdlib, or where they were already passed.

2. The use of `/` in strings for joining paths like this `pathobj / "foo/bar"` appears like it would break using the test suite on windows. Is that true? If so, do we care?

That's the beauty of it — pathlib internally uses forward slashes, so everything works as expected.

>>> from pathlib import PureWindowsPath
>>> str(PureWindowsPath(r"c:\some\directory") / "foo/bar")
'c:\\some\\directory\\foo\\bar'

@beckermr beckermr enabled auto-merge February 27, 2025 18:35
@beckermr beckermr added this pull request to the merge queue Feb 27, 2025
Merged via the queue into regro:main with commit e806e9e Feb 27, 2025
6 of 8 checks passed
@mgorny mgorny deleted the test-tmpdir-cleanup branch February 27, 2025 19:05
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.

2 participants