-
Notifications
You must be signed in to change notification settings - Fork 65
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
Switch to pytest-lazy-fixtures #756
Switch to pytest-lazy-fixtures #756
Conversation
31595a8
to
8a0863e
Compare
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.
Thanks Gigon! 🙏
Had a couple questions below
@@ -1,6 +1,6 @@ | |||
The MIT License (MIT) | |||
MIT License |
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.
Given pytest-lazy-fixtures
bundles the license file in the wheel and conda packages, do we need to bundle the license file here or could it be dropped?
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.
Thanks @jakirkham !
All files under '3rdparty' folder is actually optional and we added the folder for tracking licenses internally.
Since 'LICENSE-3rdparty.md' file on the root folder covers the list of 3rdparty libraries, do you think it is better to remove whole '3rdparty' folder instead of maintaining the folder?
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's a good idea. Let's follow up after we finish the devendoring work: #753
Since some of those dependencies are statically linked now, we do need to include their licenses. Though it is possible after devendoring that we can substantially reduce or drop licenses from this list
With Python only dependencies, we could clean those up before devendoring, but it might make sense to wait until we can do a more thorough cleanup
dependencies.yaml
Outdated
@@ -303,7 +303,7 @@ dependencies: | |||
- psutil>=5.8.0 | |||
- pytest>=6.2.4,<8.0.0a0 | |||
- pytest-cov>=2.12.1 | |||
- pytest-lazy-fixture>=0.6.3 | |||
- pytest-lazy-fixtures |
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.
Do we want a lower bound here as well?
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.
Thanks @jakirkham !
I can update it if needed.
I wonder how we can determine the lowest version to use here.
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.
Perhaps 1.0.0
is good enough to start?
Do the tests run ok with 1.0.0
?
- pytest-lazy-fixtures | |
- pytest-lazy-fixtures>=1.0.0 |
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.
Great! Thank you! Will update this.
@@ -16,7 +16,7 @@ | |||
import shutil | |||
|
|||
import pytest | |||
from pytest_lazyfixture import lazy_fixture | |||
from pytest_lazy_fixtures import lf as lazy_fixture |
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.
nice to see that this can continue to be used as-is without further changes to the test cases using the fixture
Remove the `pytest-lazy-fixture` plugin and use the replacement `pytest-lazy-fixtures` package instead. Address rapidsai#755 Signed-off-by: Gigon Bae <gbae@nvidia.com>
8a0863e
to
48668ec
Compare
/merge |
Remove the
pytest-lazy-fixture
plugin and use the replacementpytest-lazy-fixtures
package instead.Address #755