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

rewrite load tests with the pytest framework #3154

Merged
merged 2 commits into from
Apr 5, 2021

Conversation

neutrinoceros
Copy link
Member

@neutrinoceros neutrinoceros commented Mar 27, 2021

PR Summary

I'm experiencing issues with these tests running on pytest in #3062, specifically:

https://tests.yt-project.org/job/yt_py38_unittests/18/testReport/junit/yt.tests/test_load_errors/test_load_nonexistent_data/

I originally wrote these tests for pytest but had to rewrite them to get them merged, so I'm rewriting them back in hopes that will help fixing the initial problem in #3062

For reference I could not reproduce the issue locally but I assume pytest + Jenkins is less well behaved with tests using a different framework.

@neutrinoceros neutrinoceros added bug tests: running tests Issues with the test setup labels Mar 27, 2021
@neutrinoceros neutrinoceros mentioned this pull request Mar 27, 2021
5 tasks
@neutrinoceros
Copy link
Member Author

neutrinoceros commented Mar 27, 2021

Since this change alone doesn't trigger the problematic job on Jenkins (yt_py38_unittests), we should see the interesting results in #3062 instead

@neutrinoceros
Copy link
Member Author

Ah yes, this test file is actually part of the nose-based GH workflow as well, so no wonder this crashes as well.
Welp @Xarthisius, @jcoughlin11, any idea how to go around this without adding to our technical debt ? Should I just wait for nose to be fully dropped ?

@neutrinoceros
Copy link
Member Author

Note that in #3062's log we can see that it didn't fix the problem: my tests relying on tmp dir are apparently broken on fido no matter how I write them (while they work in any case locally)
https://tests.yt-project.org/job/yt_py38_unittests/23/testReport/junit/yt.tests/test_load_errors/test_load_not_a_file/

@munkm
Copy link
Member

munkm commented Mar 27, 2021

Would you mind renaming this PR to something clearer since you're only modifying one test? It's helpful when I'm writing the changelog.

@Xarthisius
Copy link
Member

@yt-fido test this please.

py38_answertests are failing cause it needs #3135

@Xarthisius
Copy link
Member

py38_unittests passes and new tests were run.

Ah yes, this test file is actually part of the nose-based GH workflow as well, so no wonder this crashes as well.
Welp @Xarthisius, @jcoughlin11, any idea how to go around this without adding to our technical debt ? Should I just wait for nose to be fully dropped ?

I'm not sure how to quantify the increase of technical debt introduced by 030b3e7 but I hope it's on the acceptable level. There's no need to wait for nose to be fully dropped.

@neutrinoceros
Copy link
Member Author

It's actually 8 tests but yeah it's only one file. I'll update the title

@neutrinoceros neutrinoceros changed the title rewrite tests with the pytest framework rewrite load tests with the pytest framework Mar 28, 2021
@neutrinoceros
Copy link
Member Author

@yt-fido test this please

@neutrinoceros neutrinoceros removed the bug label Mar 28, 2021
@neutrinoceros
Copy link
Member Author

I'm not sure how to quantify the increase of technical debt introduced by 030b3e7 but I hope it's on the acceptable level. There's no need to wait for nose to be fully dropped.

It seems reasonable to me, with the condition that the test file is still run somewhere, and that it will eventually be tested in GH workflows again. To be clear, where is that commit from ? is it part of any active PR, or were you suggesting I apply it here ?

@Xarthisius
Copy link
Member

It seems reasonable to me, with the condition that the test file is still run somewhere, and that it will eventually be tested in GH workflows again. To be clear, where is that commit from ? is it part of any active PR, or were you suggesting I apply it here ?

It's already here (see commits). I used the sneaky "feature" that allows me to remotely hack your github repo.

@Xarthisius Xarthisius added the infrastructure Related to CI, versioning, websites, organizational issues, etc label Mar 28, 2021
@neutrinoceros
Copy link
Member Author

Ok cool, fine by me. Thanks for doing this !

@neutrinoceros neutrinoceros added the refactor improve readability, maintainability, modularity label Mar 30, 2021
@munkm munkm merged commit 5eda2e0 into yt-project:main Apr 5, 2021
@neutrinoceros neutrinoceros deleted the refactor_load_test_pytest branch April 6, 2021 06:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
infrastructure Related to CI, versioning, websites, organizational issues, etc pytest refactor improve readability, maintainability, modularity tests: running tests Issues with the test setup
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants