Skip to content

Conversation

@pablogsal
Copy link
Member

@pablogsal pablogsal commented Jun 20, 2018

Copy link
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

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

The commit message can be enhanced:

  • Try to write a shorter title (first line)
  • I'm not sure that 5' unit is commonly known, I suggest to use the more common "5 min".

Example of commit message:

bpo-33716, test_concurrent_futures: increase timeout

Increase the timeout from 1 min to 5 min.

Replace also time.time() with time.monotonic() for timeouts.

You can use git push --force here to rewrite the commit message.

@bedevere-bot
Copy link

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

@pablogsal pablogsal changed the title bpo-33716: Increase timeout to 5' and use time.monotonic() in test_concurrent_futures bpo-33716: test_concurrent_futures: increase timeout Jun 20, 2018
Increase the timeout from 1 min to 5 min.

Replace also time.time() with time.monotonic() for timeouts.
@pablogsal
Copy link
Member Author

pablogsal commented Jun 20, 2018

@vstinner Thanks! I was under the assumption that all the commits will be squashed when the PR is merged and the final merged commit message will be rewritten there. ✏️

I have made the requested changes; please review again

@bedevere-bot
Copy link

Thanks for making the requested changes!

@vstinner: please review the changes made to this pull request.

vstinner
vstinner previously approved these changes Jun 20, 2018
Copy link
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

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

LGTM.

"bpo-33716: test_concurrent_futures: increase timeout"

nitpick: I use "bpo-33716, test_concurrent_futures: increase timeout" (comma, not colon), but it's really up to you. There is no written guidelines for commit messages.

@vstinner
Copy link
Member

@pitrou: I proposed to increase the timeout from 1 min to 5 min to repair buildbots, but does the check still make sense with a maximum duration of 5 minutes? https://bugs.python.org/issue33716#msg320126

@vstinner
Copy link
Member

@pablogsal: I added the "skip news" label. I don't think that a NEWS entry is needed for such simple test change. I only add a NEWS entry for major changes in tests.

Copy link
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

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

LGTM.

@pitrou wrote that he doesn't recall why I added the sanity test with a limit of 60 seconds and that "It's probably ok to increase the timeout, though.".

@pablogsal pablogsal merged commit 3ad8dec into python:master Jun 21, 2018
@pablogsal pablogsal deleted the bpo33716 branch June 21, 2018 11:30
@miss-islington
Copy link
Contributor

Thanks @pablogsal for the PR 🌮🎉.. I'm working now to backport this PR to: 3.7.
🐍🍒⛏🤖

@miss-islington
Copy link
Contributor

Thanks @pablogsal for the PR 🌮🎉.. I'm working now to backport this PR to: 3.6.
🐍🍒⛏🤖 I'm not a witch! I'm not a witch!

@bedevere-bot
Copy link

GH-8263 is a backport of this pull request to the 3.7 branch.

@miss-islington
Copy link
Contributor

Sorry, @pablogsal, I could not cleanly backport this to 3.6 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker 3ad8decd76c736f393755537aeb19b5612c21761 3.6

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Jul 12, 2018
Increase the timeout from 1 min to 5 min.

Replace also time.time() with time.monotonic() for timeouts.
(cherry picked from commit 3ad8dec)

Co-authored-by: Pablo Galindo <[email protected]>
@bedevere-bot
Copy link

GH-8264 is a backport of this pull request to the 3.6 branch.

miss-islington added a commit that referenced this pull request Jul 12, 2018
Increase the timeout from 1 min to 5 min.

Replace also time.time() with time.monotonic() for timeouts.
(cherry picked from commit 3ad8dec)

Co-authored-by: Pablo Galindo <[email protected]>
vstinner added a commit that referenced this pull request Jul 12, 2018
Increase the timeout from 1 min to 5 min.

Replace also time.time() with time.monotonic() for timeouts.

(cherry picked from commit 3ad8dec)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants