Skip to content

Add end-to-end tests to prepare for PackageFinder location sorting rewrite #5849

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

Add end-to-end tests to prepare for PackageFinder location sorting rewrite #5849

wants to merge 1 commit into from

Conversation

uranusjr
Copy link
Member

@uranusjr uranusjr commented Oct 4, 2018

Ultimate goal is #5846. Currently this only add tests to ensure the current behaviour is throughally tested (according to coverage.py).

@cjerdonek cjerdonek added C: finder PackageFinder and index related code type: refactor Refactoring code skip news Does not need a NEWS file entry (eg: trivial changes) labels Oct 10, 2018
@cjerdonek cjerdonek added this to the Internal Cleansing milestone Oct 10, 2018
@uranusjr uranusjr force-pushed the packagefinder-sort-locations-refactor branch from a878223 to 27b519c Compare October 16, 2018 07:52
@pypa-bot
Copy link

Hello!

I am an automated bot and I have noticed that this pull request is not currently able to be merged. If you are able to either merge the master branch into this pull request or rebase this pull request against master then it will eligible for code review and hopefully merging!

@pypa-bot pypa-bot added the needs rebase or merge PR has conflicts with current master label Oct 16, 2018
@uranusjr uranusjr force-pushed the packagefinder-sort-locations-refactor branch from 27b519c to 0c635e7 Compare October 16, 2018 08:01
@pypa-bot pypa-bot removed the needs rebase or merge PR has conflicts with current master label Oct 16, 2018
@uranusjr uranusjr changed the title (WIP) PackageFinder location sorting rewrite Add end-to-end tests to prepare for PackageFinder location sorting rewrite Oct 23, 2018
@uranusjr
Copy link
Member Author

I am satisfied with this now, and it’s probably best to merge this first and work on the rewrite in new PRs.

@BrownTruck
Copy link
Contributor

Hello!

I am an automated bot and I have noticed that this pull request is not currently able to be merged. If you are able to either merge the master branch into this pull request or rebase this pull request against master then it will be eligible for code review and hopefully merging!

@BrownTruck BrownTruck added the needs rebase or merge PR has conflicts with current master label May 19, 2019
@uranusjr
Add additional E2E tests to PackageFinder
b997122 
This ensures we cover all path-sorting branches, so everything designed to
work is guarenteed to work.

Old unit tests directly using _sort_locations() are replaced by E2E
tests so we can pick apart that function.
@uranusjr uranusjr force-pushed the packagefinder-sort-locations-refactor branch from 0c635e7 to b997122 Compare May 20, 2019 07:19
@pypa-bot pypa-bot removed the needs rebase or merge PR has conflicts with current master label May 20, 2019
@cjerdonek
Copy link
Member

I just saw this again. Why are some of the tests being removed when some are added?

@uranusjr
Copy link
Member Author

I think the idea is to avoid testing against PackageFinder._sort_locations() so I can reassemble it into more maintainable logic. These tests don’t actually test any sorting logic, only whether a link is parsed and accepted/rejected correctly.

Honestly I am not very sure after so long (can’t believe it’s six months ago), but that seems to explain what I did…

@cjerdonek
Copy link
Member

If you were removing the _sort_locations() tests because you want to remove _sort_locations() later, I think it would be better just to remove the tests when you remove the method. It doesn't seem like it hurts to leave them in. IMO we should only remove tests if we can confirm they're duplicates of other tests.

Re: the tests, it looks like the tests are checking that locations are being put into the right buckets ("files" or "urls"). (I think "sort" might not be the best name for this method as it's not sorting into an ordering but rather sorting in the sense of "putting into the right bucket.")

@uranusjr
Copy link
Member Author

I guess the PR is not meaningful now since I’m not working on _sort_locations() now anyway (and don’t have the capacity to do so in the immediate future). I can always dig this back out and revisit the idea when I eventually(?) come back to it.

@uranusjr uranusjr closed this May 22, 2019
@cjerdonek
Copy link
Member

Aren’t the new tests still good though, which was the main reason for the PR?

@uranusjr
Copy link
Member Author

Yes they are, but I can always add them when they are meaningful 😄 Right now _sort_locations() tests cover the same ground so they don’t add anything.

@cjerdonek
Copy link
Member

Okay, I was under the impression that the new tests added code coverage since they're end-to-end tests whereas the others are unit tests.

@lock lock bot added the auto-locked Outdated issues that have been locked by automation label Jun 24, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Jun 24, 2019
@uranusjr uranusjr deleted the packagefinder-sort-locations-refactor branch September 28, 2020 14:39
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees
No one assigned
Labels
auto-locked Outdated issues that have been locked by automation C: finder PackageFinder and index related code skip news Does not need a NEWS file entry (eg: trivial changes) type: refactor Refactoring code
Projects
None yet
Milestone
Internal Cleansing
Development

Successfully merging this pull request may close these issues.
5 participants
@uranusjr @pypa-bot @BrownTruck @cjerdonek @pradyunsg