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

Add pytest-timeout and refactor requirements.txt files #3509

Merged
merged 1 commit into from
Aug 2, 2023

Conversation

webbnh
Copy link
Member

@webbnh webbnh commented Aug 1, 2023

After some difficulties I had with a hanging test in #3501, I've decided to add a timeout mechanism to the PyTest tests. This is trivially done simply by adding a requirement for the pytest-timeout plugin and setting a timeout value in the pytest.ini file. Once this change is merged, the PyTest tests will henceforth be aborted if they run longer than 300 seconds (5 minutes). Note that affects only tests run under PyTest, which includes the unit tests and the functional tests but not the Agent "legacy" tests (which, I think, already provide timeouts). Hopefully 5 minutes is long enough for individual tests; if not, we can easily increase the limit.

In the course of making this change, I noted that we have a bunch of redundancy and similar silliness in our Python requirements files for the Agent, Client, and Server, and the "test" ones are referenced (AFAICT) only from the common tox.ini file, so there isn't much need (at the moment, anyway) to maintain strict separation between them since they are always referenced together.

Thus, rather than adding the new pytest-timeout dependency in multiple places, this PR adds it once to a new, top-level test-requirements.txt which contains all the dependencies of the testing system(s) which are common between the Agent, Client, and Server, and it removes the common requirements from the Client- and Server-specific test-requirements.txt files (the Agent-specific test-requirements.txt file ended up empty so I removed it, and so Git regards the creation of the new file as a "rename" of the Agent one), and it fixes up the tox.ini file to match.

[The PR is in Draft mode pending proof that the tests still work under the new organization.]

Copy link
Member

@dbutenhof dbutenhof left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If Jenkins is happy, this looks fine. I don't see why you didn't finish the consolidation of test requirements, but that's not a big deal.

@webbnh webbnh marked this pull request as ready for review August 1, 2023 16:14
@webbnh
Copy link
Member Author

webbnh commented Aug 1, 2023

If Jenkins is happy, this looks fine.

Mr. Jenkins does indeed appear to be happy! 🎉 (I wasn't sure that each of the functional tests would finish under the limit...that's the most likely sore spot going forward, especially if we put a "large" upload in there...and, I'm still slightly amazed that adding timeouts was as simple as adding a line in a requirements.txt file and adding another line in the pytest.ini file....)

I don't see why you didn't finish the consolidation of test requirements, but that's not a big deal.

I didn't know whether it was important to retain some level of separation or not. I mean, we could just say, "in order to test (any/all of) Pbench, you need the following dependencies" and then specify just a single file at the top level; however, I figured there might be some value in being able to say, "I'm testing just the Agent, so don't bother installing the additional dependencies which only the Server/functional-test requires" (i.e., this is what we currently do for Python 3.6). So, the new test-requirements.txt file is just the common dependencies, and it's used on both P3.6 and P3.9, whereas the additional requirements (of which, currently, only the Server has any) are used only on P3.9...which, hopefully, will save us in the event that the Server develops a dependency on something which isn't available for P3.6.

So...this is now ready for review! 😁

@webbnh webbnh requested a review from ndokos August 1, 2023 16:14
@webbnh webbnh merged commit 780adca into distributed-system-analysis:main Aug 2, 2023
@webbnh webbnh deleted the pytest_timeout branch August 2, 2023 17:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants