search

m-lab / ndt-e2e-clientworker

Code for the client worker of the NDT end-to-end test framework
Apache License 2.0
1 stars 4 forks source link

Fixing client wrapper so it always quit()s Selenium #55

Closed mtlynch closed 8 years ago

mtlynch commented 8 years ago

This fixes a bug in that the client_wrapper was just calling close() on the Selenium driver, which closes the browser window, but does not free resources associated with the browser driver. To do that, we need to call quit().

This changes create_browser to a context manager so that callers can call it in a with block that automatically calls quit() at the end. Updates the unit tests to match.

Small refactoring of apply_patches_for_create_browser so that it's now shared between both the banjo tests and the HTML5 tests.

This change is Reviewable

stephen-soltesz commented 8 years ago

When I started this review, the build reported failure here: https://github.com/m-lab/ndt-e2e-clientworker/blob/master/tests/test_http_server.py#L61

I reran the travis check three times, and it passed two of three times; I left the third in a failed state. Is that expected?

The issue appears unrelated to these changes. Is this a flaky test?

:lgtm:

Reviewed 7 of 7 files at r1. Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.

_tests/ndt_client_test.py, line 22 [r1] (raw file):_ nit: and optional, and out of scope for this PR. Based on the file name and this class name, I expected this to be a test, but it's a base class for other ndt client tests. My thought is that the name NdtClientTestCase preserves the sense that it's a test case. The file name too, ndt_client_testcase.py or ndt_client_unittest might help distinguish this file more clearly from an actual test.

Edit: though I see that the file name convention is different than I expect; test files start with test_ rather than end with _test. So, the file name may be clear in context.

Comments from Reviewable

coveralls commented 8 years ago

Coverage Status

Coverage increased (+0.02%) to 88.117% when pulling bca742f0720dd95a62ad110adf5de89fea39913a on mtlynch:quit-selenium into db73343c877b6d43bbaa7ecaf6a724c2a12929bc on m-lab:master.

mtlynch commented 8 years ago

I reran the travis check three times, and it passed two of three times; I left the third in a failed state. Is that expected?

The issue appears unrelated to these changes. Is this a flaky test?

Yeah, this is #43. It turns out my idea for fixing a race condition by introducing a smaller race condition didn't completely work. I have an idea for a complete elimination of the race, but I've been deferring until we get through the first run of M1 testing.

Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.

_tests/ndt_client_test.py, line 22 [r1] (raw file):_ Sure, I like that name better. Done.

Comments from Reviewable

coveralls commented 8 years ago

Coverage Status

Coverage increased (+0.02%) to 88.117% when pulling 32637cd51f4e8d74afad254d7b2ff7a052e74590 on mtlynch:quit-selenium into db73343c877b6d43bbaa7ecaf6a724c2a12929bc on m-lab:master.