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

Fix resources/test (when running locally) #10127

Merged
merged 2 commits into from Mar 22, 2018

Conversation

Projects
None yet
6 participants
@Hexcles
Copy link
Member

Hexcles commented Mar 21, 2018

  1. Check the HTTP port of wptserve instead of HTTPS to avoid the
    unnecessary complexities of setting up SSL context (which may fail in
    some environments, depending on the system OpenSSL version).
  2. Use exponential backoff when waiting for wptserve and specify a
    maximum retry to avoid indefinite hang.
  3. Use terminate instead of kill to give wptserve a chance to clean
    up, which is especially useful when running locally.

Fixes #10114.


This change is Reviewable

@Hexcles Hexcles requested review from jgraham and jugglinmike Mar 21, 2018

@wpt-pr-bot wpt-pr-bot added the infra label Mar 21, 2018

@wpt-pr-bot wpt-pr-bot requested review from ayg and gsnedders Mar 21, 2018

@Hexcles

This comment has been minimized.

Copy link
Member Author

Hexcles commented Mar 21, 2018

cc @lukebjerring you should be able to debug your PR locally with this patch.

@w3c-bots

This comment has been minimized.

Copy link

w3c-bots commented Mar 21, 2018

Build PASSED

Started: 2018-03-22 19:05:17
Finished: 2018-03-22 19:22:34

View more information about this build on:

@lukebjerring

This comment has been minimized.

Copy link
Contributor

lukebjerring commented Mar 21, 2018

Yep, this lets me past the issue I was having.

@Hexcles

This comment has been minimized.

Copy link
Member Author

Hexcles commented Mar 21, 2018

Fixed the SSL issue. The PR is now ready for review :)

@jugglinmike
Copy link
Contributor

jugglinmike left a comment

I can't reproduce the issue locally, but I can at least verify that this doesn't regress tox on my system.

# Exponential backoff.
wait = 1
for _ in range(5):
time.sleep(wait)

This comment has been minimized.

Copy link
@jugglinmike

jugglinmike Mar 22, 2018

Contributor

If we move this to the end of the loop body, we might avoid unnecessary startup delays. What do you think?

This comment has been minimized.

Copy link
@jugglinmike

jugglinmike Mar 22, 2018

Contributor

Also, time.sleep(_ ** 2) might make your intention more clear (along with renaming _ and removing wait)

This comment has been minimized.

Copy link
@Hexcles

Hexcles Mar 22, 2018

Author Member

I actually put it here intentionally as I noticed wptserve did take a short while to start and the first attempt often failed.

You meant 2 ** iterations right? Yeah that'd be a bit cleaner. Let me change it.

@Hexcles

This comment has been minimized.

Copy link
Member Author

Hexcles commented Mar 22, 2018

@jugglinmike thanks. A bit more details:

Creating a SSL context is actually quite tricky. Explicitly specifying TLS/SSL version isn't portable as newer OpenSSL may deprecate older, unsafe protocols (in fact, produce a fatal error). There is a new constant ssl.PROTOCOL_TLS that alleviates the issue but is only available in very recent Python. A safer way is ssl.create_default_context and then tweaks the trust settings. At the end of the day, it's just overly and unnecessarily complicated...

Hexcles added some commits Mar 21, 2018

Fix resources/test (when running locally)
1. Check the HTTP port of wptserve instead of HTTPS to avoid the
   unnecessary complexities of setting up SSL context (which may fail in
   some environments).
2. Use exponential backoff when waiting for wptserve and specify a
   maximum retry to avoid indefinite hang.
3. Use `terminate` instead of `kill` to give wptserve a chance to clean
   up, which is especially useful when running locally.

WPTServe.url still returns a HTTPS URL to make sure tests that require
secure context (e.g. service workers) still work.

@Hexcles Hexcles force-pushed the fix-resources-unittest branch from c18d546 to c6e42ae Mar 22, 2018

@Hexcles Hexcles merged commit 9dcebcb into master Mar 22, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@Hexcles Hexcles deleted the fix-resources-unittest branch Mar 22, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.