-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
Fixtures test add #3190
Fixtures test add #3190
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some minor comments. Out of curiosity, what are the tests that failed and why? If we can rewrite the test to use these fixtures without functionally altering them; that would be a great win.
tests/console/commands/test_add.py
Outdated
cachy2 = get_package("cachy", "0.2.0") | ||
msgpack_dep = get_dependency("msgpack-python", ">=0.5 <0.6") | ||
cachy2.requires = [msgpack_dep] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could replace the default cachy
package we are adding int he repo; as this represents real world where package depends on something else or has extras. Same goes for cachy1
above. Might as well add the "full" package.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the root of the problem with the test failures I was seeing.
When I put this in the fixture:
msgpack_dep = get_dependency("msgpack-python", ">=0.5 <0.6", optional=True)
cachy010 = get_package("cachy", "0.1.0")
cachy010.requires = [msgpack_dep]
repo.add_package(cachy010)
cachy020 = get_package("cachy", "0.2.0")
cachy020.requires = [msgpack_dep]
repo.add_package(cachy020)
...
Tests like test_add_constraint_with_extras
fail with:
E AssertionError: assert '\nUpdating d...chy (0.1.0)\n' == '\nUpdating d...chy (0.1.0)\n'
E Skipping 79 identical leading characters in diff, use -v to show
E - erations: 1 install, 0 updates, 0 removals
E ? ^
E + erations: 2 installs, 0 updates, 0 removals
E ? ^ +
E
E + • Installing msgpack-python (0.5.3)...
E
E ...Full output truncated (2 lines hidden), use '-vv' to show
and in the fixture if I change msgpak_dep to:
msgpack_dep = get_dependency("msgpack-python", ">=0.5 <0.6")
I get a failure of:
E poetry.repositories.exceptions.PackageNotFound: Package [msgpack-python] not found.
I wasn't sure of a way to fix this problem. Any suggestions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That failure is expected, and is probably more representative of scenarios since you added msgpack as a dependency, poetry installs it.
For the failure, you just need to add a msgpack-python
package with a compatible version to the repo.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I am getting closer now. The fixture currently looks like:
@pytest.fixture(autouse=True)
def repo_add_default_packages(repo):
repo.add_package(get_package("msgpack-python", "0.5.6"))
msgpack_dep = get_dependency("msgpack-python", ">=0.5 <0.6", optional=True)
cachy010 = get_package("cachy", "0.1.0")
cachy010.extras = {"msgpack": [get_dependency("msgpack-python")]}
cachy010.requires = [msgpack_dep]
repo.add_package(cachy010)
cachy020 = get_package("cachy", "0.2.0")
cachy020.extras = {"msgpack": [get_dependency("msgpack-python")]}
cachy020.requires = [msgpack_dep]
repo.add_package(cachy020)
repo.add_package(get_package("pendulum", "1.4.4"))
repo.add_package(get_package("cleo", "0.6.5"))
repo.add_package(get_package("tomlkit", "0.5.5"))
repo.add_package(get_package("pyyaml", "3.13"))
repo.add_package(get_package("pyyaml", "4.2b2"))
The issue I am having now is with cachy020 some of the tests expect msgpack-python to be required and some don't. So if i make it optional the ones that require it fail, but if I require it the ones that don't fail. For example test_add_constraint_dependencies
fails when msgpack-python is optional, and test_add_no_constraint
fails when it is not optional.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can modify the tests to make it work. In cases where adding the dependency means that the output now installs 2 packages (as it is also now installing the dependency), it is okay top update the output as long as it is not doing somehting unexpected.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright, I was able to get all of the tests passing using the autouse.
tests/console/commands/test_add.py
Outdated
@@ -24,10 +24,18 @@ def old_tester(tester): | |||
return tester | |||
|
|||
|
|||
def test_add_no_constraint(app, repo, tester): | |||
@pytest.fixture() | |||
def repo_with_packages(repo): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this is more an action than an object, I'd more inclined to name it something along the lines of repo_add_default_packages
or add_default_packages_to_repo
or similar. Just a personal preference.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, this makes sense. I will update.
Superseded by #9285 |
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
Relates-to: #3155
Adding a fixture for packages in the repo to test add tests. Some tests require different extras which caused the tests to fail when autouse was used on the fixture so instead it was only added where it would not cause tests to fail.
Also removed some parameters that weren't being used on certain tests.