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

Remove extras from user-supplied constraints in backtracking resolver #1808

Merged

Conversation

thomdixon
Copy link
Contributor

@thomdixon thomdixon commented Feb 3, 2023

This change ensures extras are removed from user-supplied constraints in the backtracking resolver, fixing #1806 and dependabot/dependabot-core#6550. Credit goes to @deivid-rodriguez for the suggestion.

Contributor checklist
  • Provided the tests for the changes.
  • Assure PR title is short, clear, and good to be included in the user-oriented changelog
Maintainer checklist
  • Assure one of these labels is present: backwards incompatible, feature, enhancement, deprecation, bug, dependency, docs or skip-changelog as they determine changelog listing.
  • Assign the PR to an existing or new milestone for the target version (following Semantic Versioning).

@thomdixon thomdixon changed the title Remove extras from user-supplied constraints (fixes #1806) Remove extras from user-supplied constraints in backtracking resolver (fixes #1806) Feb 3, 2023
@jeffwidman
Copy link
Contributor

jeffwidman commented Feb 3, 2023

Thanks for putting up this PR!

Given the suggestion came from Deivid and copies his patch, you might want to include him as an additional author on the commit?

Is it possible to include a test to ensure no future regression?

Co-Authored-By: David Rodríguez <deivid.rodriguez@riseup.net>
@thomdixon thomdixon force-pushed the thomdixon/strip-extras-from-upgrades branch from 92a871a to cdeaed4 Compare February 3, 2023 20:41
@thomdixon
Copy link
Contributor Author

Given the suggestion came from Deivid and copies his patch, you might want to include him as an additional author on the commit?

Thanks for the suggestion! I amended the commit and force-pushed the update.

Is it possible to include a test to ensure no future regression?

Unfortunately, there's only one unit test that uses the backtracking resolver fixture, and it's not fully fleshed out with the required options, nor an InstallCommand that has the appropriate behavior to actually exercise this code. Further, using e.g., mock.MagicMock proved difficult due to type assertions in the code.

@deivid-rodriguez
Copy link
Contributor

Yeah, I actually noticed that by removing a few related lines and seeing that no tests would fail. Hopefully we can get some guidance on how to proceed, maybe get the fix introduced for now based on our realworld testing until unit tests can be improved.

@thomdixon
Copy link
Contributor Author

thomdixon commented Feb 10, 2023

Just an update: I added an integration test for this case in order to prevent regression. Without this PR the test fails and with the PR the test passes.

@atugushev atugushev added the bug Something is not working label Feb 21, 2023
Copy link
Member

@atugushev atugushev left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@atugushev atugushev merged commit 7d22cb1 into jazzband:main Feb 22, 2023
@atugushev atugushev changed the title Remove extras from user-supplied constraints in backtracking resolver (fixes #1806) Remove extras from user-supplied constraints in backtracking resolver Feb 22, 2023
@atugushev atugushev added this to the 6.12.3 milestone Feb 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something is not working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants