NCSU Project - Random web content generator #20448

asoni3 · 2018-03-27T03:50:02Z

Here is the link to our repository which contains the web-page fuzzer: Link
File: start_servo.py
The script is intended to start servo on localhost:7002
File: servo_automation_screenshot.py
The below program is intended to test rendering mismatches in servo based on random web pages generated by the web page fuzzer.

Here is the breakdown of how our code works:

A session is started on localhost:7002
The randomly generated webpage's (html files) data is sent as JSON to this session
Using curl request, we load the html files for this session ID based on the session we just created
For each html file it renders, it takes the screenshot after rendering which is saved in the current working directory in the format "testing#.png"

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix #__ (github issue number if applicable).

There are tests for these changes OR
These changes do not require tests because _____

This change is

highfive · 2018-03-27T03:50:08Z

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

servo-wpt-sync · 2018-03-27T03:50:17Z

Opened new PR for upstreamable changes.

Completed upstream sync of web-platform-test changes at jdm/web-platform-tests#45.

jdm · 2018-03-28T15:45:37Z

cc @Manishearth so you know what's going on with this project

jdm

Good start! I've left some comments about ways the code could be cleaner and more extensible.

Please also run chmod -x tests/wpt/web-platform-tests/tools/webdriver/setup.py to revert the changes to that file. It shouldn't be necessary to make it executable in this PR.

jdm · 2018-03-28T15:51:03Z

etc/file0.html

@@ -0,0 +1,145 @@
+<!DOCTYPE html><html>


Let's not add the generated files in this pull request.

jdm · 2018-03-28T15:51:43Z

tests/wpt/web-platform-tests/tools/webdriver/build/lib.linux-x86_64-2.7/webdriver/__init__.py

@@ -0,0 +1,27 @@
+from client import Cookies, Element, Find, Session, Timeouts, Window


Let's remove the tests/wpt/web-platform-tests/tools/webdriver/{build,dist} directories from this pull request.

jdm · 2018-03-28T15:53:29Z

etc/servo_automation_screenshot.py

+"""
+Created on Mon Mar 26 20:08:25 2018
+
+@author: Pranshu Sinha, Abhay Soni, Aayushi Agrawal


This file is missing the Servo license header that is present in other files.

jdm · 2018-03-28T15:53:55Z

etc/servo_automation_screenshot.py

+"""
+
+"""
+The below program is intended to test rendering mismatches in servo based on random web pages generated by the web page fuzzer.


This program in particular is only supposed to take screenshots of arbitrary files. It doesn't need to mention the random web page generator.

jdm · 2018-03-28T15:54:44Z

etc/servo_automation_screenshot.py

+start_servo.start_servo()
+
+#Since servo takes time to load and to get the session ID for subsequent steps, we added a sleep for 60 seconds
+time.sleep(60)


This seems unnecessarily long. Can we run some commands in a loop to wait only as long as necessary until Servo returns some status indicating that it's ready?

jdm · 2018-03-28T16:09:52Z

etc/servo_automation_screenshot.py

+#Since servo takes time to load and to get the session ID for subsequent steps, we added a sleep for 60 seconds
+time.sleep(60)
+
+#The program assumes that the html files to be rendered in servo are present in the same current working directory in the format "file#.html"


Let's accept URLs or file names as command line arguments rather than making this assumption.

jdm · 2018-03-28T16:10:12Z

etc/servo_automation_screenshot.py

+
+#The program assumes that the html files to be rendered in servo are present in the same current working directory in the format "file#.html"
+cwd = os.getcwd()
+url = 'http://localhost:7002/session'


Let's avoid hardcoding this port value in more than one place if possible.

jdm · 2018-03-28T16:11:00Z

etc/servo_automation_screenshot.py

+payload = "{}"
+headers = {'content-type': 'application/json', 'Accept-Charset': 'UTF-8'}
+r = requests.post(url, data=payload, headers=headers)
+json_string = r.json()


Let's give this a more meaningful name.

This comment was not addressed.

jdm · 2018-03-28T16:11:21Z

etc/servo_automation_screenshot.py

+    json_data = json.dumps(json_data)
+    url2 = 'http://localhost:7002/session/'+json_string['value']['sessionId']+'/url'
+    payload2 = json_data
+    headers2 = {'content-type': 'application/json', 'Accept-Charset': 'UTF-8'}


Can't we reuse the previous headers if they're identical?

jdm · 2018-03-28T16:12:46Z

etc/servo_automation_screenshot.py

+    headers2 = {'content-type': 'application/json', 'Accept-Charset': 'UTF-8'}
+    r2 = requests.post(url2, data=payload2, headers=headers2)
+
+    curl_command2 = 'curl -v http://localhost:7002/session/'+json_string['value']['sessionId']+'/screenshot | jq -r ".value" | base64 -d > testing'+str(x)+'.png'


Rather than running this specific command, can we translate it into an equivalent request.get, and subsequent python library calls to get the equivalent result? Additionally, let's print some output show that the user knows where the generated screenshots are and what they're called.

servo-wpt-sync · 2018-04-03T00:42:26Z

Transplanted upstreamable changes to existing PR.

Completed upstream sync of web-platform-test changes at jdm/web-platform-tests#45.

servo-wpt-sync · 2018-04-03T01:03:42Z

No upstreamable changes; closed existing PR.

Completed upstream sync of web-platform-test changes at jdm/web-platform-tests#45.

jdm · 2018-04-03T03:00:09Z

etc/servo_automation_screenshot.py

+time.sleep(60)
+# We are waiting for reponse and we will edit as soon as we get the response.
+
+# sock = socket.socket(socket.AF_INET, socket.SOCK_STREAM)


As discussed by email, this is the right way to check for the webdriver server's availability.

jdm · 2018-04-03T03:01:58Z

etc/servo_automation_screenshot.py

+        elif opt in ("-u", "--url"):  # store the value provided with the option -u in url_cl variable.
+            global url_cl
+            url_cl = arg
+        elif opt in ("-i", "--ifile"):  # store the value provided with the option -i in inputfile variable.


Why do we have separate arguments for a URL and input file?

So the idea was to have user input localhost as URL and the location where the files are stored in input file variable. Should we always just open servo on localhost? That would mean we will just need the location of where the files are located?

What are your thoughts? @Manishearth

jdm · 2018-04-03T03:02:35Z

etc/servo_automation_screenshot.py

+session_request = requests.post(url, data=payload, headers=headers)
+json_string = session_request.json()
+# Currently, we have generated 5 random html pages, hence the loop runs for 5 iterations
+for x in range(5):


Rather than making this assumption, we should have the caller provide all of the URLs or filenames to load on the command line. There's no need to make any guesses or assumptions.

jdm · 2018-04-03T03:02:55Z

etc/servo_automation_screenshot.py

+# connected = False
+# while not connected:
+#    try:
+#        sock.connect(('localhost',7002))


Don't forget to un-hardcode the port.

jdm · 2018-04-03T03:03:20Z

etc/servo_automation_screenshot.py

+payload = "{}"
+headers = {'content-type': 'application/json', 'Accept-Charset': 'UTF-8'}
+r = requests.post(url, data=payload, headers=headers)
+json_string = r.json()


This comment was not addressed.

jdm · 2018-04-03T03:04:27Z

etc/servo_automation_screenshot.py

+    json_data = {}
+    json_data['url'] = 'file://' + file_url + 'file' + str(x) + '.html'
+    json_data = json.dumps(json_data)
+    payload2 = json_data


Why do we create this payload2 value?

jdm · 2018-04-03T03:04:48Z

etc/servo_automation_screenshot.py

+    url_request = requests.post(url + json_string['value']['sessionId'] + '/url', data=payload2, headers=headers)
+    screenshot_request = requests.get(url + json_string['value']['sessionId'] + '/screenshot')
+    image_data_encoded = screenshot_request.json()['value']
+    with open("screenshots/output_image_" + str(x) + ".png", "wb") as image_file:


Won't this fail if the screenshots directory does not exist?

jdm · 2018-04-03T03:05:29Z

etc/start_servo.py

+
+def start_servo(port, resolution):
+    cmds = ['cd ..', './mach run --webdriver ' + port + ' --resolution ' + resolution]
+    p = subprocess.Popen('/bin/bash', stdin=subprocess.PIPE, stdout=subprocess.PIPE, stderr=subprocess.PIPE)


My comments about this function from the previous review were not addressed.

jdm · 2018-04-03T03:06:30Z

etc/servo_automation_screenshot.py

+            print('python etc/servo_automation_screenshot.py -u <url> -p <port> -i <html_file_url> -r <resolution>')
+            sys.exit()
+        elif opt in ("-p", "--port"):  # store the value provided with the option -p in port variable.
+            global port


Instead of running half of the program inside of a main() function and storing values in global variables, why don't we run the entire program inside main and use local variables?

Manishearth · 2018-04-05T00:21:51Z

etc/servo_automation_screenshot.py

+process_servo = start_servo.start_servo(port, resolution)
+
+# Since servo takes time to load and to get the session ID for subsequent steps, we added a sleep for 60 seconds
+time.sleep(60)


Ideally we'd have Servo in webdriver mode output something to the console when ready so we can start immediately after

gootorov · 2018-04-06T15:10:35Z

etc/servo_automation_screenshot.py

+        sys.exit(2)
+    for opt, arg in opts:
+        if opt == '-h':  # -h means help. Displays how to input command line arguments
+            print('python etc/servo_automation_screenshot.py -u <url> -p <port> -i <html_file_url> -r <resolution>')


nit: Other print's mention python3, but this one is just python. Also, moving it to a new function like print_help() would make things easier if you want to add/change command line arguments in the future and reduce chances of making a typo.

Manishearth · 2018-04-16T01:30:59Z

etc/start_servo.py

+    cmds = ['cd ..', './mach run --webdriver ' + port + ' --resolution ' + resolution]
+    p = subprocess.Popen('/bin/bash', stdin=subprocess.PIPE, stdout=subprocess.PIPE, stderr=subprocess.PIPE)
+    for cmd in cmds:
+        p.stdin.write(cmd + "\n")


This needs a .encode('utf-8') on the passed string to work

Manishearth · 2018-04-16T01:32:04Z

etc/start_servo.py

+
+
+def start_servo(port, resolution):
+    cmds = ['cd ..', './mach run --webdriver ' + port + ' --resolution ' + resolution]


We should os.chdir() instead of calling cd, though it's unclear as to why we need to change the directory at all

asoni3 · 2018-04-24T20:50:51Z

Hi @Manishearth,
I have tried to address most of the comments, let me know if I forgot something. There is one comment that I have responded to, as soon as I get feedback, I will try to address it. The issues on the Random Web Content Generator are yet to be addressed. Let me know if you need additional changes.

I am using this command to run the file from servo:
python3 ./etc/servo_automation_screenshot.py -u localhost -p 7002 -i /path/to/folder/containing/files -r 1200x800 -n 5
Thanks!

Manishearth · 2018-04-25T04:52:34Z

etc/servo_automation_screenshot.py

+import getopt
+
+
+def servoReadyToAccept(url, payload, headers):


nit: python functions are in snake_case

Manishearth · 2018-04-25T04:52:52Z

etc/servo_automation_screenshot.py

+
+        json_data = {}
+        json_data['url'] = 'file://' + file_url + 'file' + str(x) + '.html'
+        print(json_data['url'])


remove debugging prints, or make them output some text about what it's doing

Manishearth · 2018-04-25T04:53:35Z

etc/servo_automation_screenshot.py

+        json_data['url'] = 'file://' + file_url + 'file' + str(x) + '.html'
+        print(json_data['url'])
+        json_data = json.dumps(json_data)
+        requests.post(url + '/' + json_string['value']['sessionId'] + '/url', data=json_data, headers=headers)


use a format string here

Manishearth · 2018-04-25T04:53:40Z

etc/servo_automation_screenshot.py

+        json_data = json.dumps(json_data)
+        requests.post(url + '/' + json_string['value']['sessionId'] + '/url', data=json_data, headers=headers)
+        # print('Response for html file post: ',url_request)
+        screenshot_request = requests.get(url + '/' + json_string['value']['sessionId'] + '/screenshot')


Manishearth · 2018-04-25T04:59:38Z

etc/servo_automation_screenshot.py

+            image_file.write(base64.decodebytes(image_data_encoded.encode('utf-8')))
+        print("################################")
+
+        print("The screenshot is stored in the location: " + cwd +


also here

(anywhere you use string concatenation)

Manishearth · 2018-04-25T05:01:49Z

etc/servo_automation_screenshot.py

+            file_url = arg
+        elif opt in ("-r", "--resolution"):  # store the value provided with the option -r in resolution variable.
+            resolution = arg
+        elif opt in ("-n", "--numOfFiles"):  # store the value provided with the option -n in num_of_files variable.


--num-files

Manishearth · 2018-04-25T05:02:57Z

etc/servo_automation_screenshot.py

+        elif opt in ("-p", "--port"):  # store the value provided with the option -p in port variable.
+            port = arg
+        elif opt in ("-u", "--url"):  # store the value provided with the option -u in url variable.
+            url = arg


Do we need this argument? Seems like url will always be localhost.

(We can also make port default to 8080 or something)

asoni3 · 2018-04-25T18:02:35Z

Hi @Manishearth,

I have addressed the comments. Let me know any additional changes you might want me to do.

Thanks!

Manishearth

minor changes, then we'll land. Please also squash your commits

Manishearth · 2018-04-25T21:49:14Z

etc/servo_automation_screenshot.py

+        json_data['url'] = 'file://{0}file{1}.html'.format(file_url, str(x))
+        print(json_data['url'])
+        json_data = json.dumps(json_data)
+        requests.post('{0}/{1}/url'.format(url, json_string['value']['sessionId']), data=json_data, headers=headers)


Leave some comments with links to webdriver docs about these endpoints and what's going on

Manishearth · 2018-04-27T16:35:41Z

etc/servo_automation_screenshot.py

+    except getopt.GetoptError:
+        # an error is generated if the options provided in commandline are wrong.
+        # The help statement is printed on how to input command line arguments.
+        print('Enter the correct command to run this file')


both of these should output some info on usage (expected flags, etc). Just saying "Usage: ./etc/servo_automation_screenshot.py filename -p portnumber" or whatever the format is would be enough

Manishearth · 2018-04-27T17:56:25Z

etc/servo_automation_screenshot.py

+if __name__ == "__main__":
+    if len(sys.argv) < 8:
+        print('Enter the correct command to run this file')
+        print('python3 ./etc/servo_automation_screenshot.py -p <port>'


This usage print should be a function so that you don't have to copy paste it

asoni3 · 2018-04-27T18:53:29Z

Hi @Manishearth,

Sorry for that, I hope there aren't anymore problems. Let me know.

Thanks!

Manishearth · 2018-04-27T18:55:40Z

@bors-servo r+

looking good!

Your next steps are to get this working with Firefox, yes?

bors-servo · 2018-04-27T18:55:40Z

📌 Commit 437276d has been approved by Manishearth

asoni3 · 2018-04-27T18:57:48Z

@Manishearth Yes! We are in progress, you can review it from here. Let us know what are your thoughts on this. We are almost done with fixing the issues in that repository. Will push the code soon.
Thanks!

bors-servo · 2018-04-27T19:14:54Z

⌛ Testing commit 437276d with merge a3fc009...

NCSU Project - Random web content generator  Here is the link to our repository which contains the web-page fuzzer: [Link](https://github.com/asoni3/Random-web-content-generator---CSC-517-OSS-Project) **File: start_servo.py** The script is intended to start servo on localhost:7002 **File: servo_automation_screenshot.py** The below program is intended to test rendering mismatches in servo based on random web pages generated by the web page fuzzer. Here is the breakdown of how our code works: * A session is started on localhost:7002 * The randomly generated webpage's (html files) data is sent as JSON to this session * Using curl request, we load the html files for this session ID based on the session we just created * For each html file it renders, it takes the screenshot after rendering which is saved in the current working directory in the format "testing#.png" ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [ ] These changes fix #__ (github issue number if applicable).  - [ ] There are tests for these changes OR - [ ] These changes do not require tests because _____    --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/20448)

bors-servo · 2018-04-27T19:42:50Z

💔 Test failed - linux-rel-wpt

asoni3 · 2018-04-27T20:50:48Z

Hi @Manishearth,

I tried looking at the logs and I am not able to understand why is this failing. I am not sure if this is related to my work. Can you please take a look?

Thanks!

Manishearth · 2018-04-27T20:52:10Z

@bors-servo retry

something might be wrong with the CI

bors-servo · 2018-04-27T20:52:15Z

⌛ Testing commit 437276d with merge 625634a...

NCSU Project - Random web content generator  Here is the link to our repository which contains the web-page fuzzer: [Link](https://github.com/asoni3/Random-web-content-generator---CSC-517-OSS-Project) **File: start_servo.py** The script is intended to start servo on localhost:7002 **File: servo_automation_screenshot.py** The below program is intended to test rendering mismatches in servo based on random web pages generated by the web page fuzzer. Here is the breakdown of how our code works: * A session is started on localhost:7002 * The randomly generated webpage's (html files) data is sent as JSON to this session * Using curl request, we load the html files for this session ID based on the session we just created * For each html file it renders, it takes the screenshot after rendering which is saved in the current working directory in the format "testing#.png" ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [ ] These changes fix #__ (github issue number if applicable).  - [ ] There are tests for these changes OR - [ ] These changes do not require tests because _____    --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/20448)

bors-servo · 2018-04-27T23:40:15Z

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css1, mac-rel-css2, mac-rel-wpt1, mac-rel-wpt2, mac-rel-wpt3, mac-rel-wpt4, windows-msvc-dev
Approved by: Manishearth
Pushing 625634a to master...

asoni3 · 2018-04-28T00:36:12Z

Hi @Manishearth,

Thank you. Could you also review the random web content generator repository and check if we are on the right track?

Thanks!

highfive assigned asajeffrey Mar 27, 2018

highfive added the S-awaiting-review There is new code that needs to be reviewed. label Mar 27, 2018

servo-wpt-sync mentioned this pull request Mar 27, 2018

NCSU Project - Random web content generator jdm/web-platform-tests#45

Closed

jdm assigned jdm and unassigned asajeffrey Mar 27, 2018

jdm requested changes Mar 28, 2018

View reviewed changes

jdm added S-needs-code-changes Changes have not yet been made that were requested by a reviewer. and removed S-awaiting-review There is new code that needs to be reviewed. labels Mar 28, 2018

highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-needs-code-changes Changes have not yet been made that were requested by a reviewer. labels Apr 3, 2018

jdm reviewed Apr 3, 2018

View reviewed changes

jdm requested changes Apr 3, 2018

View reviewed changes

jdm added S-needs-code-changes Changes have not yet been made that were requested by a reviewer. and removed S-awaiting-review There is new code that needs to be reviewed. labels Apr 3, 2018

jdm assigned Manishearth and unassigned jdm Apr 3, 2018

Manishearth reviewed Apr 5, 2018

View reviewed changes

gootorov reviewed Apr 6, 2018

View reviewed changes

Manishearth reviewed Apr 16, 2018

View reviewed changes

highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-needs-code-changes Changes have not yet been made that were requested by a reviewer. labels Apr 24, 2018

Manishearth requested changes Apr 25, 2018

View reviewed changes

Manishearth approved these changes Apr 25, 2018

View reviewed changes

asoni3 force-pushed the master branch 2 times, most recently from f0a9299 to b277eaa Compare April 27, 2018 04:28

Manishearth reviewed Apr 27, 2018

View reviewed changes

asoni3 force-pushed the master branch 2 times, most recently from f9be855 to 0b20543 Compare April 27, 2018 16:55

Manishearth reviewed Apr 27, 2018

View reviewed changes

Automation Script to take screenshot for servo

437276d

asoni3 force-pushed the master branch from 0b20543 to 437276d Compare April 27, 2018 18:05

highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-awaiting-review There is new code that needs to be reviewed. labels Apr 27, 2018

highfive added S-tests-failed The changes caused existing tests to fail. and removed S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. labels Apr 27, 2018

highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-tests-failed The changes caused existing tests to fail. labels Apr 27, 2018

bors-servo merged commit 437276d into servo:master Apr 27, 2018

highfive removed the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Apr 27, 2018

		@@ -0,0 +1,27 @@
		from client import Cookies, Element, Find, Session, Timeouts, Window



		def start_servo(port, resolution):
		cmds = ['cd ..', './mach run --webdriver ' + port + ' --resolution ' + resolution]

NCSU Project - Random web content generator #20448

NCSU Project - Random web content generator #20448

Conversation

asoni3 commented Mar 27, 2018 • edited

highfive commented Mar 27, 2018

servo-wpt-sync commented Mar 27, 2018

jdm commented Mar 28, 2018

jdm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

servo-wpt-sync commented Apr 3, 2018

servo-wpt-sync commented Apr 3, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asoni3 commented Apr 24, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asoni3 commented Apr 25, 2018

Manishearth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Manishearth Apr 27, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asoni3 commented Apr 27, 2018

Manishearth commented Apr 27, 2018

bors-servo commented Apr 27, 2018

asoni3 commented Apr 27, 2018 • edited

bors-servo commented Apr 27, 2018

bors-servo commented Apr 27, 2018

asoni3 commented Apr 27, 2018

Manishearth commented Apr 27, 2018

bors-servo commented Apr 27, 2018

bors-servo commented Apr 27, 2018

asoni3 commented Apr 28, 2018

asoni3 commented Mar 27, 2018 •

edited

asoni3 commented Apr 24, 2018 •

edited

Manishearth Apr 27, 2018 •

edited

asoni3 commented Apr 27, 2018 •

edited