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

pytest4: use tmpdir fixture #51

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

keszybz
Copy link
Member

@keszybz keszybz commented Aug 2, 2019

No description provided.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.3%) to 73.502% when pulling d2bc845 on keszybz:pytest4-compat into a39fb50 on mdp-toolkit:master.

@Stewori
Copy link
Member

Stewori commented Aug 7, 2019

Hey @keszybz, would you drop us a note what this is fixing and how?

@Stewori
Copy link
Member

Stewori commented Aug 7, 2019

Okay, I found http://doc.pytest.org/en/latest/tmpdir.html#the-tmpdir-fixture
This is just some code cleanup without fixing a specific problem, right?
I am fine with merging this, just wanted to understand it.

@Stewori
Copy link
Member

Stewori commented Aug 7, 2019

Looked at the Travis results and several turned red. Some of them might be separate issues, e.g. related to #50, i.e. those failing due to hang in test_SFANode.
Given that there is no compelling reason for this change (is there?) I would postpone it until we understand the failures better.

@Debilski
Copy link
Member

Debilski commented Aug 7, 2019

The pytest tmpdir object uses the __fspath__ protocol for compatibility with the os.path.join function (and others), but this protocol has only been introduced in Python 3.6, so it breaks on 3.5 unless you also rewrite the join functions. ( https://docs.python.org/3/whatsnew/3.6.html#pep-519-adding-a-file-system-path-protocol )

@Stewori
Copy link
Member

Stewori commented Aug 8, 2019

Okay, thanks for the clarification. So, I do not see a sufficiently compelling reason for the change that justifies breaking Python 3.5 compatibility. Maybe a fallback would be feasible, but I neither see a sufficiently compelling reason to invest the work to get this change in. @keszybz If you see a reasonably efficient solution, feel free to adjust the PR such that travis turns green. Then I will happily accept it.

@keszybz
Copy link
Member Author

keszybz commented Aug 14, 2019

I was updating the Fedora package for mdp for new compatibility with newest pytest and python3.8. I got some error related to this code. It seemed simplest to get rid of the custom code and use the tmpdir fixture. Nevertheless, now that I rested this, the error was most likely caused by something else, and current code in master works correctly with python3.8b3 and pytest-4.6.4. So this patch is not necessary. I think it'd make sense to merge after python3.5 compatiblity is not required anymore.

@cclauss cclauss mentioned this pull request Feb 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants