{"action":"created","issue":{"url":"https://api.github.com/repos/m-lab/ndt-e2e-clientworker/issues/2","repository_url":"https://api.github.com/repos/m-lab/ndt-e2e-clientworker","labels_url":"https://api.github.com/repos/m-lab/ndt-e2e-clientworker/issues/2/labels{/name}","comments_url":"https://api.github.com/repos/m-lab/ndt-e2e-clientworker/issues/2/comments","events_url":"https://api.github.com/repos/m-lab/ndt-e2e-clientworker/issues/2/events","html_url":"https://github.com/m-lab/ndt-e2e-clientworker/pull/2","id":142371720,"number":2,"title":"First Part of Selenium Driver for NDT HTML5 Client","user":{"login":"rowanv","id":7131826,"avatar_url":"https://avatars.githubusercontent.com/u/7131826?v=3","gravatar_id":"","url":"https://api.github.com/users/rowanv","html_url":"https://github.com/rowanv","followers_url":"https://api.github.com/users/rowanv/followers","following_url":"https://api.github.com/users/rowanv/following{/other_user}","gists_url":"https://api.github.com/users/rowanv/gists{/gist_id}","starred_url":"https://api.github.com/users/rowanv/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/rowanv/subscriptions","organizations_url":"https://api.github.com/users/rowanv/orgs","repos_url":"https://api.github.com/users/rowanv/repos","events_url":"https://api.github.com/users/rowanv/events{/privacy}","received_events_url":"https://api.github.com/users/rowanv/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"milestone":null,"comments":10,"created_at":"2016-03-21T14:47:26Z","updated_at":"2016-03-24T16:09:28Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/m-lab/ndt-e2e-clientworker/pulls/2","html_url":"https://github.com/m-lab/ndt-e2e-clientworker/pull/2","diff_url":"https://github.com/m-lab/ndt-e2e-clientworker/pull/2.diff","patch_url":"https://github.com/m-lab/ndt-e2e-clientworker/pull/2.patch"},"body":"This PR includes the following: \r\n1. Initial implementation of the test sequence with the Selenium driver\r\n2. Error handling for test timeout, with corresponding unit tests\r\n3. Error handling for failure to load the landing page, with corresponding UTs,\r\n4. Error handling for missing numeric values, with corresponding UTs.\n\n<!-- Reviewable:start -->\n---\nThis change is [<img src=\"https://reviewable.io/review_button.svg\" height=\"35\" align=\"absmiddle\" alt=\"Reviewable\"/>](https://reviewable.io/reviews/m-lab/ndt-e2e-clientworker/2)\n<!-- Reviewable:end -->\n"},"comment":{"url":"https://api.github.com/repos/m-lab/ndt-e2e-clientworker/issues/comments/200904867","html_url":"https://github.com/m-lab/ndt-e2e-clientworker/pull/2#issuecomment-200904867","issue_url":"https://api.github.com/repos/m-lab/ndt-e2e-clientworker/issues/2","id":200904867,"user":{"login":"rowanv","id":7131826,"avatar_url":"https://avatars.githubusercontent.com/u/7131826?v=3","gravatar_id":"","url":"https://api.github.com/users/rowanv","html_url":"https://github.com/rowanv","followers_url":"https://api.github.com/users/rowanv/followers","following_url":"https://api.github.com/users/rowanv/following{/other_user}","gists_url":"https://api.github.com/users/rowanv/gists{/gist_id}","starred_url":"https://api.github.com/users/rowanv/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/rowanv/subscriptions","organizations_url":"https://api.github.com/users/rowanv/orgs","repos_url":"https://api.github.com/users/rowanv/repos","events_url":"https://api.github.com/users/rowanv/events{/privacy}","received_events_url":"https://api.github.com/users/rowanv/received_events","type":"User","site_admin":false},"created_at":"2016-03-24T16:09:28Z","updated_at":"2016-03-24T16:09:28Z","body":"\n\n\n\nReview status: all files reviewed at latest revision, 14 unresolved discussions.\n\n---\n\n*[client_wrapper/client_wrapper.py, line 93 \\[r3\\]](https://reviewable.io:443/reviews/m-lab/ndt-e2e-clientworker/2#-KDcPiLiWxvNvoTghIbE:-KDcqingpbdF8STjMIgh:-2003896356) ([raw file](https://github.com/m-lab/ndt-e2e-clientworker/blob/ac9046cbefc1bbe78a35a0635465c88c70f9cb34/client_wrapper/client_wrapper.py#L93)):*\nThanks, fixed.\n\n---\n\n*[client_wrapper/client_wrapper.py, line 95 \\[r3\\]](https://reviewable.io:443/reviews/m-lab/ndt-e2e-clientworker/2#-KDcPod65UAjF7eXblcE:-KDdEvDgV2i3BkgzYSLj:1874996301) ([raw file](https://github.com/m-lab/ndt-e2e-clientworker/blob/ac9046cbefc1bbe78a35a0635465c88c70f9cb34/client_wrapper/client_wrapper.py#L95)):*\nSounds good. Fixed so uses `NdtResult` object throughout, removed the `_` prefix.\n\n---\n\n*[client_wrapper/client_wrapper.py, line 111 \\[r3\\]](https://reviewable.io:443/reviews/m-lab/ndt-e2e-clientworker/2#-KDcTYPFXFB7u-YmoPtf:-KDdGUsxNwbC6Ga8GBhy:-1412990965) ([raw file](https://github.com/m-lab/ndt-e2e-clientworker/blob/ac9046cbefc1bbe78a35a0635465c88c70f9cb34/client_wrapper/client_wrapper.py#L111)):*\nAh, ok. Sure, I went ahead and moved the click start test button logic into its own function. With regards to the other steps for the test, though, I wanted to make sure I'm understanding the general idea --  at each step we are catching errors and adding them to `result.errors`, but their presence triggers a return of `perform_test` in these remaining (the `WebDriverException` and the `TimeoutException`). If we move the try/except logic into a sub-function that undertakes part of the testing process, then each of these sub-functions would also need to return a variable that would indicate whether or not that sub-function had been successful/the larger `perform_test` should return its results? Or am I misunderstanding how that logic would work? Thanks.\n\n---\n\n*[client_wrapper/client_wrapper.py, line 143 \\[r3\\]](https://reviewable.io:443/reviews/m-lab/ndt-e2e-clientworker/2#-KDcQGctrUmNwDpbIowk:-KDcrT-y_15itpE88NeU:1718834792) ([raw file](https://github.com/m-lab/ndt-e2e-clientworker/blob/ac9046cbefc1bbe78a35a0635465c88c70f9cb34/client_wrapper/client_wrapper.py#L143)):*\nSure, fixed.\n\n---\n\n*[client_wrapper/client_wrapper.py, line 199 \\[r3\\]](https://reviewable.io:443/reviews/m-lab/ndt-e2e-clientworker/2#-KDcZPEMMIPggmIJyzgS:-KDcrqmLX6_KKjZ9suBo:-374393068) ([raw file](https://github.com/m-lab/ndt-e2e-clientworker/blob/ac9046cbefc1bbe78a35a0635465c88c70f9cb34/client_wrapper/client_wrapper.py#L199)):*\nSure, removed backticks here and in other places where they surrounded variable names in comments.\n\n---\n\n*[client_wrapper/client_wrapper.py, line 202 \\[r3\\]](https://reviewable.io:443/reviews/m-lab/ndt-e2e-clientworker/2#-KDcZJn5990lYrAPH4kh:-KDcs6wxpi2x0ECLlseH:-2003896356) ([raw file](https://github.com/m-lab/ndt-e2e-clientworker/blob/ac9046cbefc1bbe78a35a0635465c88c70f9cb34/client_wrapper/client_wrapper.py#L202)):*\nThanks, fixed.\n\n---\n\n*[client_wrapper/client_wrapper.py, line 221 \\[r3\\]](https://reviewable.io:443/reviews/m-lab/ndt-e2e-clientworker/2#-KDccPh_I78Y5QbwWAlE:-KDcsGS3ThxCG73KDIlT:-1962606206) ([raw file](https://github.com/m-lab/ndt-e2e-clientworker/blob/ac9046cbefc1bbe78a35a0635465c88c70f9cb34/client_wrapper/client_wrapper.py#L221)):*\nSounds good, fixed.\n\n---\n\n*[client_wrapper/client_wrapper.py, line 236 \\[r3\\]](https://reviewable.io:443/reviews/m-lab/ndt-e2e-clientworker/2#-KDcZtBV4Yn8cmy2LoPZ:-KDcsccwT53vxoAm38wH:1250065474) ([raw file](https://github.com/m-lab/ndt-e2e-clientworker/blob/ac9046cbefc1bbe78a35a0635465c88c70f9cb34/client_wrapper/client_wrapper.py#L236)):*\nSounds good, changed.\n\n---\n\n*[client_wrapper/client_wrapper.py, line 245 \\[r3\\]](https://reviewable.io:443/reviews/m-lab/ndt-e2e-clientworker/2#-KDc_-O3gOu0iOYgtY1M:-KDcsoQ_Z31Uro5bQseZ:-1536417447) ([raw file](https://github.com/m-lab/ndt-e2e-clientworker/blob/ac9046cbefc1bbe78a35a0635465c88c70f9cb34/client_wrapper/client_wrapper.py#L245)):*\nsounds good, changed docstring.\n\n---\n\n*[tests/test_client_wrapper.py, line 22 \\[r2\\]](https://reviewable.io:443/reviews/m-lab/ndt-e2e-clientworker/2#-KDZ6l0lu58WcmxFFgeY:-KDdJPyBKSStfrmKWGCI:-1454966916) ([raw file](https://github.com/m-lab/ndt-e2e-clientworker/blob/a8a1d15a2c3986b37383a4eda0a462aa1c799373/tests/test_client_wrapper.py#L22)):*\nSure, so the most clearcut example is the difference between `NdtHtml5SeleniumDriverInvalidURLTest` and `NdtHtml5SeleniumDriverGeneralTest`. The former has our mock driver act like a normal driver throughout our application, then the second just has our mock driver is throw a `WebDriverException` as a side effect when it's first instantiated. So if we just patch everything by patching `client_wrapper.webdriver.Firefox`, then maybe we could just change `self.mock_driver.return_value = NewDriver()` within `test_invalid_URL_throws_error` -- but we can't, because  we've already called the patcher's `start` method. So then maybe we just define the patch in the `setUp`, but actually `start` it within each individual unit test, but I'm not sure that the extra work of defining it in every method would make sense, given that we'd only be able to consolidate 1 additional test into our general class.\n\nThen with `NdtHtml5SeleniumDriverCustomClassTest` we have that explicit class creation in order to be able to individually test result values. So we could make it a part of `NdtHtml5SeleniumDriverGeneralTest`, but then wed have to define the `mock_driver` within each individual unit test, so as to not override the explicit class creation in the tests in `NdtHtml5SeleniumDriverCustomClassTest`. Let me know if you think there is a clearer solution, thanks.\n\n---\n\n*[tests/test_client_wrapper.py, line 107 \\[r3\\]](https://reviewable.io:443/reviews/m-lab/ndt-e2e-clientworker/2#-KDc_skw4PfVJKccz2YY:-KDd20_32rA5855m49q0:862311698) ([raw file](https://github.com/m-lab/ndt-e2e-clientworker/blob/ac9046cbefc1bbe78a35a0635465c88c70f9cb34/tests/test_client_wrapper.py#L107)):*\nSounds good, I went ahead and added a new test for it.\n\n---\n\n*[tests/test_client_wrapper.py, line 175 \\[r3\\]](https://reviewable.io:443/reviews/m-lab/ndt-e2e-clientworker/2#-KDcajFRgUhpNES5Rxq9:-KDct--J-nDgx5ZOONmH:-1962606206) ([raw file](https://github.com/m-lab/ndt-e2e-clientworker/blob/ac9046cbefc1bbe78a35a0635465c88c70f9cb34/tests/test_client_wrapper.py#L175)):*\nSounds good, fixed.\n\n---\n\n\n*Comments from the [review on Reviewable.io](https://reviewable.io:443/reviews/m-lab/ndt-e2e-clientworker/2)*\n<!-- Sent from Reviewable.io -->\n"}}