Automated tests for WebDriver

[krunal3103]

Created two test files in /tests/webdriver directory.
The command ./mach test-webdriver executes all the tests.py files in this directory.

---

This change is

[highfive]

Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @mbrubeck (or someone else) soon.

[frewsxcv]

Review status: 0 of 4 files reviewed at latest revision, 3 unresolved discussions.

---

tests/webdriver/test.py, line 3 [r3] (raw file):
Not everyone's "servo" directory lives in ~, so this probably won't work.

---

tests/webdriver/test.py, line 18 [r3] (raw file):
This should be indented four spaces.

---

tests/webdriver/test.py, line 28 [r3] (raw file):
This should be indented four spaces.

---

Comments from the review on Reviewable.io

[jdm]

Thanks for making this pull request! It's a good start, but I've proposed some changes that will make it easier to write more complicated tests going forwards, I think. In particular, I've made a pull request to your branch to add a standalone HTTP server, which will allow us to write tests that don't use real websites. I'd like to see what this PR looks like once we removed some of the code and test duplication; I think it will be easier to read at that point :)

-S-awaiting-review +S-needs-code-changes

---

Reviewed 4 of 5 files at r3, 3 of 3 files at r4.
Review status: all files reviewed at latest revision, 12 unresolved discussions.

---

python/servo/testing_commands.py, line 106 [r3] (raw file):
I think this is a rebase error? This testsuite doesn't exist any longer on master.

---

python/servo/testing_commands.py, line 294 [r3] (raw file):
nit: space after ,

---

python/servo/testing_commands.py, line 298 [r3] (raw file):
Test harnesses have certain requirements that invoking python via Popen doesn't quite satisfy. I've proposed a pull request for your branch that uses execfile instead so we can catch exceptions - we'll need to fill in the TODO blocks to report on tests passing or failing, and return an appropriate value to if any tests fail.

---

python/servo/testing_commands.py, line 378 [r3] (raw file):
I think we can get rid of this; I removed the caller in my branch.

---

tests/dromaeo/dromaeo, line 1 [r3] (raw file):
This looks unintentional and should be reverted.

---

tests/webdriver/test.py, line 3 [r3] (raw file):
Additionally, we should move this into the test-webdriver code that invokes python so it doesn't need to be replicated in every test.

---

tests/webdriver/test.py, line 19 [r3] (raw file):
What happened to using ServoProcess? We should move that into the harness directory I create in my branch and use it in all the tests instead of duplicating this all the time.

---

tests/webdriver/test.py, line 1 [r4] (raw file):
Can we give this file a more descriptive name?

---

tests/webdriver/test.py, line 22 [r4] (raw file):
Rather than using a real website, we should use the standalone HTTP server I included in my pull request.

---

tests/webdriver/test.py, line 33 [r4] (raw file):
I think this test is duplicating the previous one in terms of things we're actually testing about webdriver, so I don't think it's useful to keep around.

---

tests/webdriver/test1.py, line 9 [r3] (raw file):
Why duplicate ServoProcess?

---

tests/webdriver/test1.py, line 23 [r4] (raw file):
What are we testing that's different than the previous file here?

---

Comments from the review on Reviewable.io

[bors-servo]

☔ The latest upstream changes (presumably #10112) made this pull request unmergeable. Please resolve the merge conflicts.

[jdm]

-S-awaiting-review -S-needs-rebase +S-needs-code-changes

---

Reviewed 2 of 2 files at r5, 3 of 3 files at r7.
Review status: all files reviewed at latest revision, 10 unresolved discussions.

---

tests/webdriver/test.py, line 3 [r3] (raw file):
I'm pretty sure this change makes it impossible to run the tests unless the Servo code is present in a subdirectory of the root of the partition.

---

Comments from the review on Reviewable.io

[jdm]

@krunal3103 What's the plan here? I haven't seen any updates to address most of the review comments so far.

[jdm]

It looks like you've created the test_url.py to address my comments about the other python files, but those files should now be removed. I still have serious concerns about the test harness that haven't been addressed:

• the test still relies on real HTTP servers that are out of our control
• the test runner doesn't do any reporting about whether tests passed or failed
• it should be possible to share ServoProcess between all the tests, so it shouldn't be defined in a test file

-S-awaiting-review +S-needs-code-changes

---

Reviewed 1 of 1 files at r8.
Review status: all files reviewed at latest revision, 11 unresolved discussions.

---

tests/webdriver/test_url.py, line 19 [r8] (raw file):
This isn't used.

---

Comments from Reviewable

[jdm]

-S-awaiting-review +S-needs-code-changes

---

Reviewed 2 of 4 files at r9, 1 of 1 files at r10.
Review status: all files reviewed at latest revision, 8 unresolved discussions, some commit checks failed.

---

python/servo/testing_commands.py, line 298 [r3] (raw file):
Does execfile throw an exception? Is it something else that doesn't work?

---

tests/webdriver/test_url.py, line 12 [r9] (raw file):
If we use the code based on execfile, it should be enough to append to sys.path in the caller, like you said.

---

tests/webdriver/test_url.py, line 25 [r9] (raw file):
I don't know off the top of my head. I'm ok with leaving this with a comment explaining the problem.

---

tests/wpt/harness/wptrunner/executors/ServoProcess.py, line 1 [r10] (raw file):
I just noticed that this file is under tests/wpt/harness - we shouldn't make changes to this directory, since it's imported from https://github.com/w3c/wptrunner . This file should live somewhere in tests/webdriver/ instead.

---

Comments from Reviewable

[krunal3103]

We have pushed this new commit in which we have used the dummy HttpServer instead of actual URLs. We are trying to run the web driver tests using ./mach test-webdriver. We have introduced the relative paths due to which it isn't able to find certain dependencies required by the test files, in this case test_url.py
Pasting the error here

======================================================================
ERROR: test_get (mach.commands.f7840c870be911e6b707f45c8998d8e1.TestStringMethods)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/krunal/Git/servo/tests/webdriver/test_url.py", line 27, in test_get
    with ServoProcess():
NameError: global name 'ServoProcess' is not defined

----------------------------------------------------------------------
Ran 1 test in 0.000s

FAILED (errors=1)
Test passed

[jdm]

Interesting. I just checked out your branch, ran ./mach test-webdriver and got this:

godot2:dev-servo jdm$ ./mach test-webdriver
in here test_url.py
Test passed

[jdm]

My debugging shows me that test_url.py is being executed, but the if __name__ == "__main__" doesn't succeed so the testcase never runs.

[krunal3103]

The previous test_url.py does not execute the test_get method when called using execfile. I`ve pushed a new version, you can try running it to see the error.

[jdm]

For some reason the top-level imports aren't available when executing the testcases. If I add the from ServoProcess import ServoProcess inside test_get that error goes away.

[jdm]

It's not clear to me that the unittest package is adding value here. If the python files simply consist of import statements and the actual test code to be executed (using assert() instead of self.assertEquals, etc.), I feel like these problems will disappear :)

[srm09]

Referring the latest commit, we plan to move forward this way i.e. by adding the test files and raising the appropriate exceptions based on the test that is being run.
In the test-runner, we will perform the following tasks:

1. Catch the exceptions raised by tests.
2. Keep a count of the number of passed and failed test cases.
3. Print a summary of the failed test cases.

[jdm]

That sounds good to me. Don't forget to return 0 or 1 from the test runner depending on whether all tests passed or not!

[krunal3103]

For the subsequent step to check response type, I do not see an already existing method in the webdriver.py that returns that. I found the text command under class Element that can be used to check the content. But how do we get the response type? Do I need to implement that method?

[jdm]

That refers to the return value of send_command, which is the JSON response body. The goal is to ensure that it matches the expected JSON response body defined in the specification (ie. all expected fields are present, and no unexpected fields are present).

[jdm]

Good improvements! Please be sure to run ./mach test-tidy before pushing changes, and also address the old comments that are still pending in this review.
-S-awaiting-review +S-fails-tidy +S-needs-code-changes

---

Reviewed 1 of 6 files at r11, 8 of 9 files at r15.
Review status: 8 of 9 files reviewed at latest revision, 9 unresolved discussions, some commit checks failed.

---

python/servo/testing_commands.py, line 293 [r15] (raw file):
We should get rid of these if the tests are working now.

---

tests/webdriver/test_content.py, line 16 [r15] (raw file):
Let's add a comment explaining what this test is testing.

---

tests/webdriver/test_content.py, line 26 [r15] (raw file):
Why raise a different exception? Can't we have an assert without any try/except?

---

tests/webdriver/test_emptyurl.py, line 23 [r15] (raw file):
What are we testing here exactly?

---

tests/webdriver/test_emptyurl.py, line 27 [r15] (raw file):
What's the purpose of this commented code?

---

tests/webdriver/harness/test_timeout.py, line 28 [r15] (raw file):
This test doesn't really tell us anything useful right now about the webdriver implementation. https://w3c.github.io/webdriver/webdriver-spec.html#sessions says that sessions have a "session page load timeout" - what we'd like to test is that the webdriver server times out after an appropriate amount of time and follows step 6.3 of https://w3c.github.io/webdriver/webdriver-spec.html#go . We should be able to set the page load timeout via https://w3c.github.io/webdriver/webdriver-spec.html#set-timeout to make the test take less than 300 seconds :)

---

tests/wpt/harness/wptrunner/executors/webdriver.py, line 423 [r15] (raw file):
Let's revert the changes to this file.

---

Comments from Reviewable

[srm09]

Please ignore. Rebased the branch with upstream/master

[highfive]

New code was committed to pull request.

[jdm]

Due to the unaddressed review comments as well as the existence of webdriver tests in https://github.com/w3c/web-platform-tests/, giving us a shared set of tests with other browsers, I'm going to go ahead and close this. Thanks for doing this work!