Closed Bug 1283044 Opened 6 years ago Closed 6 years ago

Fix all flake8 error in testing/talos for python3

Categories

(Testing :: Talos, defect)

defect
Not set
normal

Tracking

(firefox50 fixed)

RESOLVED FIXED
mozilla50
Tracking Status
firefox50 --- fixed

People

(Reporter: malayaleecoder, Assigned: malayaleecoder, Mentored)

Details

(Whiteboard: [talos_wishlist])

Attachments

(2 files, 3 obsolete files)

Attached file flake8_fails.txt
Since we are planning to move to python3, it would be nice to clear up the flake8 fails. Please find attached the list of conflicts.
Assignee: nobody → malayaleecoder
Status: NEW → ASSIGNED
Whiteboard: [talos_wishlist]
Attached patch Bug1283044_v1.diff (obsolete) — Splinter Review
Woah, that was a lengthy one.
:jmaher, I might have done some careless mistakes :P
Do have a look.
Attachment #8766446 - Flags: review?(jmaher)
Comment on attachment 8766446 [details] [diff] [review]
Bug1283044_v1.diff

Review of attachment 8766446 [details] [diff] [review]:
-----------------------------------------------------------------

a few nits to address, also we will need to adjust the flake8 config to use testing/talos instead of testing/talos/talos.

I am excited this patch has come so far so fast!

::: testing/talos/INSTALL.py
@@ +42,5 @@
>      if virtualenv:
>          call([virtualenv, '--system-site-packages', here])
>      else:
> +        process = subprocess.Popen([sys.executable, '-', '--system-site-packages', here],
> +                                   stdin=subprocess.PIPE)

please split all arguments so they are one argument per line, and they are indented to follow the opening (

::: testing/talos/tests/test_browser_output.py
@@ +89,5 @@
>  
>          garbage = "hjksdfhkhasdfjkhsdfkhdfjklasd"
>          input = self.end_report() + garbage + self.start_report()
> +        self.compare_error_message(input, "End token '%s' occurs before start token" %
> +                                   self.end_report())

please split the arguments up on new lines here.

::: testing/talos/tests/test_talosconfig.py
@@ +8,3 @@
>  ffox_path = 'test/path/to/firefox'
> +command_args = [ffox_path, '-profile', 'pathtoprofile', '-tp', 'pathtotpmanifest', '-tpchrome',
> +                '-tpmozafterpaint', '-tpnoisy', '-rss', '-tpcycles', '1', '-tppagecycles', '1']

if you are splitting this, make each line a different item in the array.

@@ +52,5 @@
>          yaml = YAML()
>          content = yaml.read(browser_config['bcontroller_config'])
> +        self.validate(content['command'], "test/path/to/firefox -profile pathtoprofile -tp " +
> +                      "pathtotpmanifest -tpchrome -tpmozafterpaint -tpnoisy -rss -tpcycles 1 " +
> +                      "-tppagecycles 1")

same here, each argument should be on it's own line and split up the command a bit more so it is on a few lines.

@@ +68,5 @@
> +        self.validate(content['repository'], "http://hg.mozilla.org/releases/mozilla-release")
> +        self.validate(content['title'], "qm-pxp01")
> +        self.validate(content['testname'], "tp5n")
> +        self.validate(content['xperf_providers'], ['PROC_THREAD', 'LOADER', 'HARD_FAULTS',
> +                                                   'FILENAME', 'FILE_IO', 'FILE_IO_INIT'])

split this up in a few more lines.
Attachment #8766446 - Flags: review?(jmaher) → review+
Attached patch Bug1283044_v2.diff (obsolete) — Splinter Review
All done, do let me know if any more changes are required :)
Attachment #8766446 - Attachment is obsolete: true
Attachment #8767083 - Flags: review?(jmaher)
Comment on attachment 8767083 [details] [diff] [review]
Bug1283044_v2.diff

Review of attachment 8767083 [details] [diff] [review]:
-----------------------------------------------------------------

I rarely get to r-, so why not!  A treat for today.  One issue, then please push to try to see if flake8 works.  I would also run tests for linux64 and windows 7 (as you have been doing normally)

::: testing/talos/tests/test_talosconfig.py
@@ +21,5 @@
> +                '1']
> +with open("test_talosconfig_browser_config") as json_browser_config:
> +    browser_config = json.load(json_browser_config)
> +    json_browser_config.close()
> +with open("test_talosconfig_test_config") as json_test_config:

can you 'hg add test_talosconfig_browser_config' to the patch?  same for test_talosconfig_test_config?  I would prefer if these were .json files as well.
Attachment #8767083 - Flags: review?(jmaher) → review-
Attached patch Bug1283044_v3.diff (obsolete) — Splinter Review
Oops, my bad in not adding the JSON files.
I guess this should resolve it. Fingers crossed
Attachment #8767083 - Attachment is obsolete: true
Attachment #8767111 - Flags: review?(jmaher)
Comment on attachment 8767111 [details] [diff] [review]
Bug1283044_v3.diff

Review of attachment 8767111 [details] [diff] [review]:
-----------------------------------------------------------------

one nit to fix, lets seen how the try run goes.

::: testing/talos/tests/test_talosconfig.py
@@ +20,5 @@
> +                '-tppagecycles',
> +                '1']
> +with open("test_talosconfig_browser_config.json") as json_browser_config:
> +    browser_config = json.load(json_browser_config)
> +    json_browser_config.close()

you don't need to close() when using the 'with' operator.
Attachment #8767111 - Flags: review?(jmaher) → review+
The try is all green :)
I have incorporated the said changes. Please have a look.
Attachment #8767111 - Attachment is obsolete: true
Attachment #8767229 - Flags: review?(jmaher)
Comment on attachment 8767229 [details] [diff] [review]
Bug1283044_v4.diff

Review of attachment 8767229 [details] [diff] [review]:
-----------------------------------------------------------------

thanks!
Attachment #8767229 - Flags: review?(jmaher) → review+
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ce5c5ebc122b
Fix all flake8 error in testing/talos for python3. r=jmaher
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/ce5c5ebc122b
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in before you can comment on or make changes to this bug.