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

Testing the new resolver (test suite, CI) #7871

Closed
pfmoore opened this issue Mar 18, 2020 · 14 comments
Closed

Testing the new resolver (test suite, CI) #7871

pfmoore opened this issue Mar 18, 2020 · 14 comments
Labels
C: dependency resolution About choosing which dependencies to install

Comments

@pfmoore
Copy link
Member

pfmoore commented Mar 18, 2020

Now that we have at least the start of the new resolver in place, we need to consider how we integrate it into the test suite. I've modified one of the very basic install tests to run twice, once with the legacy resolver and once with --unstable-feature=resolver, but this obviously isn't a scalable approach.

Some issues we therefore need to consider:

  1. How do we have a more general mechanism that allows us to run tests with both old and new resolvers?
  2. How do we handle tests that use feaures the new resolver doesn't have implemented yet (effectively xfail for the new resolver, but not for the current one)?
  3. How do we cater for tests where we expect different results? (Tests that rely on quirks of the legacy resolver, explicit tests of new resolver functionality, ...)

Ultimately, we want the full test suite to be run for both resolvers, so that we have a good regression test ensuring that we have feature parity, but we're likely a good way from that position yet. But the only way to know for sure how much there still is to do is to (reasonably regularly) run the full suite with the new resolver.

@pfmoore pfmoore added C: dependency resolution About choosing which dependencies to install UX User experience related labels Mar 18, 2020
@pradyunsg
Copy link
Member

pradyunsg commented Mar 19, 2020

One of the things to make sure of is, that there are YAML tests that are being skipped because they don't work with the legacy resolver. We'd want to unskip them.

@pfmoore
Copy link
Member Author

pfmoore commented Jun 18, 2020

Is there anything more we need to do here? @pradyunsg @uranusjr

We currently have:

  1. A flag --new-resolver that allows us to run the tests against the new resolver.
  2. A way of marking tests that are known to still fail with the new resolver (maybe we want to repurpose this issue to track getting the number of tests marked like this down to zero?)
  3. A fixture that says "using the new resolver" that allows us to cater for differences in behaviour in the tests.

These test features will have to be modified once the new resolver becomes default, and ripped out when we get rid of the old resolver, but I'd suggest that those activities are better done as independent issues once they are needed.

If we are going to track the number of remaining "known failures" here, we should start linking issues for each of the remaining types of failure from this one, and keeping a running total here, for visibility.

@pfmoore
Copy link
Member Author

pfmoore commented Jun 18, 2020

For information, the following are the outstanding "known failures":

FAILED tests/functional/test_install.py::test_install_nonlocal_compatible_wheel_path - AssertionError: Script returned code: 1
FAILED tests/functional/test_install.py::test_install_pip_does_not_modify_pip_when_satisfied[True-install_args1-Requirement already up-to-date: pip in] - Assert...
FAILED tests/functional/test_install.py::test_install_pip_does_not_modify_pip_when_satisfied[False-install_args1-Requirement already up-to-date: pip in] - Asser...
FAILED tests/functional/test_install_reqs.py::test_install_distribution_union_with_versions - AssertionError: Script returned code: 1
FAILED tests/functional/test_install.py::test_hashed_install_failure_later_flag - AssertionError: Could not find 'Hashes are required in --require-hashes mode, ...
FAILED tests/functional/test_install_upgrade.py::test_upgrade_with_newest_already_installed - AssertionError: Looking in links: file:///C:/Users/Gustav/AppData/...
FAILED tests/functional/test_install_upgrade.py::test_only_if_needed_does_not_upgrade_deps_when_satisfied - AssertionError: did not print correct message for no...
FAILED tests/functional/test_uninstall_user.py::Tests_UninstallUserSite::test_uninstall_from_usersite_with_dist_in_global_site - AssertionError: Script returned...
FAILED tests/functional/test_install_user.py::Tests_UserSite::test_install_user_conflict_in_globalsite - AssertionError: Script returned code: 1
FAILED tests/functional/test_install_user.py::Tests_UserSite::test_install_user_conflict_in_globalsite_and_usersite - AssertionError: Script returned code: 1
FAILED tests/functional/test_install_user.py::Tests_UserSite::test_upgrade_user_conflict_in_globalsite - AssertionError: Script returned code: 1
FAILED tests/functional/test_install_upgrade.py::test_upgrade_to_same_version_from_url - AssertionError: INITools 0.3 reinstalled same version

@pradyunsg
Copy link
Member

I think the YAML tests need to be updated to respect --new-resolver, but other than that, I think this looks good to go to me! ^.^

@pradyunsg
Copy link
Member

There's still a bunch of message-printing failures. :)

@pfmoore
Copy link
Member Author

pfmoore commented Jun 18, 2020

You mean in that list I posted? I know, I keep planning on fixing them (or at least acknowledging that we print something different and moving on) but getting side-tracked with the harder cases :-)

I'll aim at easy stuff today and see if that helps 😉

@pfmoore
Copy link
Member Author

pfmoore commented Jun 18, 2020

#8462 sorts out test_install_nonlocal_compatible_wheel_path.

@pradyunsg
Copy link
Member

Well, how about you tackle the difficult stuff and I do the easy bits? 🙈

@pfmoore
Copy link
Member Author

pfmoore commented Jun 18, 2020

@pradyunsg Can you take a look at my comment on #8462? I swear I tried to pick a simple one, and it turned complicated. I think I'm jinxed... 🙄

@pfmoore
Copy link
Member Author

pfmoore commented Jun 18, 2020

Found some straightforward ones (I hope): #8466 fixes 4 more tests.

@brainwane
Copy link
Contributor

Per conversation in last week's meeting this is a tracking issue for the remaining tests that are still failing, and needs to be finished before we switch pip to defaulting to the new resolver. As of last week, the count of fails_on_new_resolver in master is down to 3 failures, and all are actively being addressed.

@nlhkabu nlhkabu removed the UX User experience related label Jul 28, 2020
@brainwane
Copy link
Contributor

Per conversation today with @pradyunsg I'm closing this as finished. Thanks all!

@uranusjr
Copy link
Member

uranusjr commented Oct 8, 2020

Would it be a good idea after 20.3 to flip the current setup, testing the new resolver by default on all platforms, and the legacy resolver only on Travis?

@pradyunsg
Copy link
Member

I'd say before. :)

I'm considering that a part of #8937.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 9, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
C: dependency resolution About choosing which dependencies to install
Projects
None yet
Development

No branches or pull requests

5 participants