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

[WIP] AppVeyor: Timeout tests after 5 minutes #4595

Closed
wants to merge 4 commits into from

Conversation

pradyunsg
Copy link
Member

Idea inspired by #4589 holding up the AppVeyor build queue for a long time.

@pfmoore Thoughts?


I feel this is simpler than #4591 and better serves our purpose (preventing long builds from holding up the build queue) by timing out the test process itself in 5 minutes on one job.

Since the tests currently finish in around 2 minutes, the timeout has a buffer of 150% of the current run time, which I think is a safe enough size to know that something got stuck.

@pradyunsg
Copy link
Member Author

Oh, great, forgot to mention what it does - this PR causes the tests to timeout in 5 minutes; if the tests don't finish in 5 minutes, the test process is killed.

@pradyunsg pradyunsg added C: automation Automated checks, CI etc type: maintenance Related to Development and Maintenance Processes skip news Does not need a NEWS file entry (eg: trivial changes) labels Jul 4, 2017
@pradyunsg
Copy link
Member Author

Wait, this creates a new window and output goes there. Oops!

@ghost
Copy link

ghost commented Jul 4, 2017

Ahh but we cannot have any improvements to the appveyor scripts because that would be too complex for pip developers.

@pfmoore
Copy link
Member

pfmoore commented Jul 4, 2017

@xoviat I don't think that was called for :-(

@pradyunsg Any reason we couldn't use something like https://pypi.python.org/pypi/pytest-timeout (found from a quick search, I've not used it myself) to do this, rather than relying on Powershell scripting?

Looking at https://ci.appveyor.com/project/pypa/pip/build/1.0.1161/job/m3bh97wv3vb2q10n I don't see the test output. So there's definitely a problem with what you currently have...

@dstufft
Copy link
Member

dstufft commented Jul 4, 2017

We already use pytest-timeout in tox, though I don't know if appveyor is calling the test runner manually instead of using tox.

@pfmoore
Copy link
Member

pfmoore commented Jul 4, 2017

Both Appveyor and Travis use tox and hence should be using the settings in tox.ini - specifically commands = py.test --timeout 300 []. So I guess the question is, if we're setting that timeout, why are we still getting stuck tests?

OTOH, it may be that --timeout 300 means 5 minutes maximum per test, whereas we're talking here about 5 minutes for the whole run. So if pytest-timeout doesn't have a means to limit the time of the whole run, maybe we do need something else?

I'm not opposed to the Powershell approach, but it needs fixing to actually work first :-)

@dstufft
Copy link
Member

dstufft commented Jul 4, 2017

Oh yea, that's a per test timeout.

@pradyunsg
Copy link
Member Author

So there's definitely a problem with what you currently have...

Powershell isn't my strongest suite. I'll give it another pass now.

Any reason we couldn't use something like https://pypi.python.org/pypi/pytest-timeout (found from a quick search, I've not used it myself) to do this, rather than relying on Powershell scripting?

We already do it; also, I imagine it won't prevent the case where a regression in pip {PR made changes that broke stuff} causes a hang in tox.

@pradyunsg
Copy link
Member Author

@dstufft Could I get access to AppVeyor so that I can cancel builds?

@dstufft
Copy link
Member

dstufft commented Jul 5, 2017

I'm not sure that I have access to AppVeyor. I'll see if I do though and if so give you access. That might require someone else though, @pfmoore maybe?

@dstufft
Copy link
Member

dstufft commented Jul 5, 2017

I don't see any way to add a new user, so I think maybe I don't have permission.

@pradyunsg
Copy link
Member Author

I just realised that I can do the testing of how AppVeyor behaves on my fork. I don't need the permissions on this repository... I guess it will just be a nice-to-have now.

@pfmoore
Copy link
Member

pfmoore commented Jul 5, 2017

I'll take a look.

@pfmoore
Copy link
Member

pfmoore commented Jul 5, 2017

See #3948 (comment)

I've added @pradyunsg as an Appveyor admin. And I'll ask the question again, do we have a better place to hold the appveyor pypa account details than just with me? (They weren't actually needed in this case, but I guess might be at some point).

@dstufft
Copy link
Member

dstufft commented Jul 5, 2017

@pfmoore We don't currently :(

@pradyunsg
Copy link
Member Author

I've added @pradyunsg as an Appveyor admin.

@pfmoore I still don't see a "Re-Build Commit" option like I do on pradyunsg/pip. And I don't see any indication that this happened over on AppVeyor. :/

@pradyunsg
Copy link
Member Author

But I do see pypa projects listed when I go to add a project. Interesting. :|

@pradyunsg
Copy link
Member Author

Anyway, thanks for adding me @pfmoore but it seems I still don't have the permissions or am missing something.

I haven't clicked anything on my end yet and I guess I can make do with the CI builds on my fork alone for now. I think you can remove me as an admin on AppVeyor - it's not something I think I'll be using.

@pfmoore
Copy link
Member

pfmoore commented Jul 5, 2017

OK. I just noticed that nor do I have "Re-build commit" when logged in as myself. Although the "pypa/Appveyor Admins" team is set up as having the Administrator role in Appveyor, so it should have worked.

I'm not going to worry about it for now, though, as you have a workaround.

@pradyunsg pradyunsg changed the title AppVeyor: Timeout tests after 5 minutes [WIP] AppVeyor: Timeout tests after 5 minutes Jul 6, 2017
@pfmoore
Copy link
Member

pfmoore commented Jul 7, 2017

Just for reference, to get the "restart build" button (if you're in the Appveyor Admins group) you need to do the following:

  1. Click on the "details" link for the Appveyor CI in a PR
  2. This takes you to appveyor, but you're not signed in. Click the Sign In link at the top right. If you are signed in, you may need to sign out and sign in again (to pick the right account, see below).
  3. Use the "Github" button to sign in with github.
  4. You'll get "Selected developer belongs to multiple accounts" - from the dropdown choose the "pypa" account and hit the github button again.
  5. You're now logged in, but you're at the pypa account's home page, not the PR build. Best way to get to the build page is to close the tab, and click the "Details" link again (appveyor will remember your login details, but it seems to only do so for a relatively short while).

While you're signed in under the pypa account, you don't have admin access to your own projects. You need to switch accounts as needed.

Personally, I find all that pretty complicated and confusing, but according to the appveyor guys it's working as designed.

@pradyunsg
Copy link
Member Author

I find all that pretty complicated and confusing

Same. It's much easier for me to just test on pradyunsg/pip itself so that's what I'm gonna do later as well.

@pradyunsg pradyunsg self-assigned this Aug 6, 2017
@pradyunsg
Copy link
Member Author

Closing this in favour of #4657 -- once that merges, this becomes fairly redundant.

@pradyunsg pradyunsg closed this Aug 13, 2017
@pradyunsg pradyunsg deleted the appveyor/timeout-builds branch August 23, 2017 05:32
@pradyunsg pradyunsg removed their assignment Oct 5, 2017
@lock
Copy link

lock bot commented Jun 2, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot added the auto-locked Outdated issues that have been locked by automation label Jun 2, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Jun 2, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
auto-locked Outdated issues that have been locked by automation C: automation Automated checks, CI etc skip news Does not need a NEWS file entry (eg: trivial changes) type: maintenance Related to Development and Maintenance Processes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants