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

Timeout functional tests #449

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

kgaughan
Copy link
Member

If the functional tests don't exist after five seconds of waiting on them, they should be killed. This also uses pytest-timeout to impose a maximum runtime of 30secs on any test in the test suite.

Hopefully this should prevent tests from hanging.

If the functional tests don't exist after five seconds of waiting on
them, they should be killed. This also uses pytest-timeout to impose a
maximum runtime of 30secs on any test in the test suite.

Hopefully this should prevent tests from hanging.
@kgaughan
Copy link
Member Author

Closer. The functional tests look to be completing, but pytest (or tox) looks to get stuck, so the final step in the workflow doesn't exit. My best guess is that there's a wait or join being executed in the atexit handler that's preventing the process from exiting.

@kgaughan
Copy link
Member Author

I just reran the failed workflows, and they succeeded. I guess it's at least better that the tests themselves aren't hanging and, but I'm not at all sure why the process sometimes doesn't exit like it should

@digitalresistor
Copy link
Member

Not a fan of this change. I feel like it papers over waitress not existing cleanly. I want tests to hang.

The GitHub actions runners have been slow and having issues for the last couple of days, locally these tests always succeed correctly.

@kgaughan
Copy link
Member Author

In that case, I can drop the changes to tests/test_functional.py and keep the timeout on the test: it should fail on the offending test while it's waiting on the join() and wait() calls to return. Sound good?

@digitalresistor
Copy link
Member

What do we gain? I am not sure I fully understand what the point of it is, today with the current test suite. The additional dependency/package is an additional one to download/install, across all CI runs. While it may be small it adds up, and I try to be cognizant of the overhead of adding yet another thing that can cause issues because it is not maintained/not updated as quickly as other parts of the ecosystem.

If the functional tests hanging was a bigger issue and one that could be triggered regularly I wouldn't be opposed to adding extra security against it.

@kgaughan
Copy link
Member Author

What I was originally trying to do was to see if it was reveal which of the functional tests was the most problematic when it came to hanging in the hopes of making the test suite more reliable. They seemed to most consistently fail on EchoTests and TooLargeTests with the timeout in place.

If you don't see it as worthwhile, I can close the PR though.

@digitalresistor
Copy link
Member

Where do you see them hanging? I am trying to understand better because I too want the test suite to not fail/and understand the issues and fix them.

While working on some stuff lately to fix some of the security issues I have re-ran the test suite 10's of times, I haven't seen it hang at all, and my test suite runs have been ~14 seconds (with coverage enabled, couple seconds shorter without coverage).

@digitalresistor
Copy link
Member

I will note that there seems to be an issue with the Github actions, the test suite there historically has ran in about ~60 seconds at most, and now I am seeing runs that are a couple of minutes a piece, or even longer.

I don't know how to account for those changes in runtime, it's the same code base, same tests.

@kgaughan
Copy link
Member Author

Yep, it confused me too. It hangs for me randomly from time to time in the middle of the functional tests. I couldn't detect a pattern to it, hence why I tried cleaning up the fixture subprocesses more aggressively.

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.

2 participants