Skip to content
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

utils test coverage improvements #2283

Merged
merged 28 commits into from
Oct 17, 2023

Conversation

sr-murthy
Copy link
Contributor

@sr-murthy sr-murthy commented Sep 27, 2023

Addresses #2273

Pull Request Checklist

  • A news fragment is added in news/ describing what is new.
  • [Y ] Test cases added for changed code.

Describe what you have changed in this PR.

  • Update pyproject.toml to add a test coverage entrypoint
  • Add tests for pdm.utils

@codecov
Copy link

codecov bot commented Sep 27, 2023

Codecov Report

All modified lines are covered by tests ✅

Files Coverage Δ
src/pdm/utils.py 92.12% <100.00%> (+7.36%) ⬆️

... and 18 files with indirect coverage changes

📢 Thoughts on this report? Let us know!.

@sr-murthy sr-murthy force-pushed the 2273-utils-test-coverage branch from 634df8d to 2e880be Compare September 27, 2023 23:34
@sr-murthy sr-murthy changed the title Update project TOML to add test coverage entrypoint + add tests for `… Test coverage improvements Sep 27, 2023
@sr-murthy sr-murthy changed the title Test coverage improvements WIP: Test coverage improvements Sep 27, 2023
@sr-murthy sr-murthy force-pushed the 2273-utils-test-coverage branch from d9f082a to 5742c09 Compare September 28, 2023 06:19
@sr-murthy sr-murthy force-pushed the 2273-utils-test-coverage branch from d0e2b67 to f78dc84 Compare September 28, 2023 14:45
@sr-murthy sr-murthy force-pushed the 2273-utils-test-coverage branch from ce2f514 to bafd974 Compare September 30, 2023 19:46
@sr-murthy sr-murthy changed the title WIP: Test coverage improvements WIP: utils test coverage improvements Oct 1, 2023
@sr-murthy sr-murthy changed the title WIP: utils test coverage improvements utils test coverage improvements Oct 3, 2023
@sr-murthy
Copy link
Contributor Author

sr-murthy commented Oct 3, 2023

@frostming This PR is complete, including the news fragment. It adds 8% to code coverage in utils (latest measurement is 88%), and adds 1% to global code coverage. This is branch-based measurement, so untested branches in existing tests are considered missing. There were some untested functions in utils which I didn't touch, because I wasn't sure about them. Also, I've also left all pre-existing tests alone. I suppose there could be a follow up PR later to address the gap.

So existing code coverage on main for utils and the global coverage is:

---------- coverage: platform darwin, python 3.11.2-final-0 ----------
Name                                               Stmts   Miss Branch BrPart  Cover   Missing
----------------------------------------------------------------------------------------------
...
src/pdm/utils.py                                     256     44     88     16    80%   37-38, 48, 90, 106, 117, 120-121, 124-125,
132->139, 135->139, 151-153, 191-195, 210, 262-263, 286, 292, 296, 299, 314-316, 321->320, 373-374, 403, 405-412, 431-436, 456-457
----------------------------------------------------------------------------------------------
TOTAL                                              10200   1427   4567    505    83%

With this PR the measurement is:

---------- coverage: platform darwin, python 3.11.2-final-0 ----------
Name                                               Stmts   Miss Branch BrPart  Cover   Missing
----------------------------------------------------------------------------------------------
...
src/pdm/utils.py                                     257     29     88      8    88%   38-39, 49, 91, 107, 152-154, 194, 211, 315-
317, 322->321, 404, 406-413, 432-437, 457-458
----------------------------------------------------------------------------------------------
TOTAL                                              10201   1412   4567    497    84%

Copy link
Collaborator

@frostming frostming left a comment

Choose a reason for hiding this comment

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

Cool, LGTM 🎉

@frostming frostming linked an issue Oct 9, 2023 that may be closed by this pull request
Copy link
Collaborator

@frostming frostming left a comment

Choose a reason for hiding this comment

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

Please fix the test failures on older Python versions and Windows.

src/pdm/utils.py Outdated
for item in items:
new_items.extend([item, sep])
return new_items[:-1]
return list(iter_chain.from_iterable(iter_product(items, [sep])))[:-1]
Copy link
Collaborator

Choose a reason for hiding this comment

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

This brings more burden to understand, I prefer to keep the old code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, no problem. Will revert

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These test failures relate to path processing, which obviously works differently on Windows and POSIX. I'll need to revisit some of the test data here.

Copy link
Contributor Author

@sr-murthy sr-murthy Oct 14, 2023

Choose a reason for hiding this comment

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

@frostming I think I've fixed the test failures. There were two issues here: (1) incompatibility in test path instantiation between POSIX and Windows platforms which affected utils.get_venv_like_prefix and utils.path_to_url, (2) incompatibility in path equality checking between pre-Python 3.12.x and Python 3.12.x versions which affected utils.get_venv_like_prefix.

I have Python 3.12.a01 installed via pyenv, but was unable to install PDM(2.9.3) due to an error, so could not test the changes locally in 3.12. But they should now work 🤞

Copy link
Collaborator

Choose a reason for hiding this comment

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

path comparison is still failing. Thanks

@sr-murthy
Copy link
Contributor Author

Will be pushing an update in a few days, hopefully will fix all of the Windows failures.

@sr-murthy
Copy link
Contributor Author

OK, I might just skip tests for path_to_url on Windows.

@sr-murthy
Copy link
Contributor Author

Tests updated to skip some for Windows.


def test_non_windows_localhost_local_file_url(self):
with mock.patch("pdm.utils.sys.platform", "non_windows"):
assert utils.url_to_path("file://localhost/local/file/path") == "/local/file/path"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe skip it on windows and add another one for Windows only.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Np.

Copy link
Contributor Author

@sr-murthy sr-murthy Oct 16, 2023

Choose a reason for hiding this comment

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

OK, I've added a single Windows-only test case for utils.url_to_path testing the conversion of a localhost local file URL to a Windows path. I hope this works - I don't use Windows and don't have access to a Windows Python environment, so it is tricky for me to test this. But I am expecting that on Windows this URL -> path conversion is valid via utils.url_to_path:

file://localhost/local/file/path -> C:/local/file/path

[?]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Amended the expected path here, trying again.

@sr-murthy sr-murthy force-pushed the 2273-utils-test-coverage branch from db85d66 to 286376b Compare October 16, 2023 12:44
tests/test_utils.py Show resolved Hide resolved
tests/test_utils.py Show resolved Hide resolved
sr-murthy and others added 2 commits October 16, 2023 14:51
fix failing `url_to_path` test cases

Co-authored-by: Frost Ming <[email protected]>
tweak `utils.url_to_path`test case skipping

Co-authored-by: Frost Ming <[email protected]>
@sr-murthy
Copy link
Contributor Author

@frostming Assuming this now passes, should I rebase some commits to squash/tidy up?

tests/test_utils.py Outdated Show resolved Hide resolved
tests/test_utils.py Outdated Show resolved Hide resolved
@frostming
Copy link
Collaborator

@frostming Assuming this now passes, should I rebase some commits to squash/tidy up?

We will squash when merge

@frostming frostming merged commit aa21abe into pdm-project:main Oct 17, 2023
frostming added a commit that referenced this pull request Oct 17, 2023
* Update project TOML to add test coverage entrypoint + add tests for `pdm.utils.create_tracked_tempdir`

* Add tests for `pdm.utils.get_trusted_hosts`

* Add tests for `pdm.utils.url_without_fragments`

* Add tests for `pdm.utils.join_list_with`

* Add a parameterised fixture for repository configs + update tests for `pdm.utils.get_trusted_hosts`

* Add tests for `pdm.utils.get_user_email_from_git`

* Add tests for `pdm.utils.add_ssh_scheme_to_git_uri`

* Add tests for `pdm.utils.path_to_url`

* Add tests for `pdm.utils.path_replace`

* Add tests for `pdm.utils.is_path_relative_to`

* Fix test name typo for `utils.expand_env_vars_in_auth`

* Add tests for `utils.get_venv_like_prefix`

* Add tests for `utils.get_rev_from_url`

* Correct test case input for `utils.add_ssh_scheme_to_uri`

* Add tests for `utils.url_to_path`

* Add tests for `utils.normalize_name`

* Add tests for `utils.is_editable`

* Add news fragment for issue #2273

* Revert changes to `pdm.utils.join_list_with`

* Amend tests for `pdm.utils.get_venv_like_prefix` to make them platform- and Python-compatible

* Amend tests for `pdm.utils.path_to_url` to make them platform-compatible

* Tweak `conftest._PathFactory.get_py_compatible_mock_path` - amend Python version check

* Tweak some `utils` tests to skip on Windows + minor refactoring of some fixtures

* Fix path fixtures in `conftest` + add Windows-only test case for `utils.url_to_path`

* Update tests/test_utils.py

fix failing `url_to_path` test cases

Co-authored-by: Frost Ming <[email protected]>

* Update tests/test_utils.py

tweak `utils.url_to_path`test case skipping

Co-authored-by: Frost Ming <[email protected]>

* Apply suggestions from code review

---------

Co-authored-by: Frost Ming <[email protected]>
Signed-off-by: Frost Ming <[email protected]>
@j178 j178 mentioned this pull request Apr 3, 2024
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.

Test coverage
2 participants