If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Ensure string formatting is using `format()` instead of `%` for all Marionette py files

RESOLVED FIXED in Firefox 51

Status

Testing
Marionette
RESOLVED FIXED
a year ago
11 months ago

People

(Reporter: whimboo, Assigned: Francesco Pischedda, Mentored)

Tracking

({good-first-bug})

Version 3
mozilla52
good-first-bug
Points:
---

Firefox Tracking Flags

(firefox51 fixed, firefox52 fixed)

Details

(Whiteboard: [lang=py], URL)

MozReview Requests

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(4 attachments)

(Reporter)

Description

a year ago
There are a lot of instances where the old `%` string formatting is used. We should update all py files under /testing/marionette to use the format() method.

Example:

Old: 'no test found: %s' % self.tests
New: 'no test found: {}'.format(self.tests)

In case we have multiple placeholders we should add numbers like `{0} .. {1}`.
(Reporter)

Updated

a year ago
Created attachment 8777871 [details]
py-files-under-testing-marionette

I am interested in fixing this bug 

a quick ls gave this list of files in the attachment should I fix the issue in all the files or just in testing/marionette ?
Flags: needinfo?(dhanvicse)
Flags: needinfo?(dhanvicse)
mentioned need more info as myself by mistake
Flags: needinfo?(hskupin)
(Reporter)

Comment 3

a year ago
Hi Tummala. Thank you for the interest to work on this bug! I'm looking forward to your patch, and once it has been uploaded will also assign the bug to you.

Regarding the list of files I don't have the time to completely compare it but it looks fine to me. What I would suggest is that you make use of mozreview to upload your patches and have different commits for each part of Marionette (client, harness, and tests). 

When you want to test your changes please follow the instructions here:
https://developer.mozilla.org/en-US/docs/Mozilla/QA/Marionette/Running_Tests

In case of any other questions please don't hesitate to ask here or join us on IRC in the #automation channel.

Thanks
Flags: needinfo?(hskupin)

Comment 4

a year ago
Hi,
Newbie here,I believe I can try some moves on this bug.
But I am not able to figure out from where and how to setup the project on my local machine?
Thanks in advance.
(Reporter)

Comment 5

a year ago
Tummala, are you still working on this bug? If yes please let us know about. If not, I would like to and over this bug to Jaspreet. Thanks.
Flags: needinfo?(dhanvicse)
(Reporter)

Comment 6

a year ago
Jaspreet, if you are still interested to work on this bug please get started. Looks like Tummala is not around anymore.

To get started I would suggest you read through the following URL:
https://wiki.mozilla.org/User:Mjzffr/New_Contributors

Comment 7

a year ago
On it,I will revert as soon as I finish over that article.

Comment 8

a year ago
Help needed.
Using Mac OS X,After cloning mozilla-central from mercurial,when I run ./mach-build I get

 0:02.64 ERROR: Only clang/llvm 3.6 or newer is supported.
 0:02.66 *** Fix above errors and then restart with               "/Applications/Xcode.app/Contents/Developer/usr/bin/make -f client.mk build"
 0:02.66 make[2]: *** [configure] Error 1
 0:02.66 make[1]: *** [/Users/jaspreet/src/mozilla-central/obj-x86_64-apple-darwin15.4.0/Makefile] Error 2
 0:02.66 make: *** [build] Error 2

So I tried checking with

brew install llvm --with-clang
Warning: llvm-3.8.1 already installed

But the error with ./mach build remains same.
(Reporter)

Comment 9

a year ago
Did you run the python/mozboot/bin/bootstrap.py script? Did it report any failures?

Btw. you do not necessarily have to build Firefox your self. The mach command for marionette-test accepts the --binary option now, which let you specify eg. a downloaded Nightly build of Firefox.

Comment 10

a year ago
Created attachment 8783468 [details]
errors.txt

Comment 11

a year ago
Comment on attachment 8783468 [details]
errors.txt

 4:48.93 TEST_START: MainThread test_accessiblecaret_selection_mode.py AccessibleCaretSelectionModeTestCase.test_long_press_changes_focus_from_textarea_to_contenteditable
 4:49.40 TEST_END: MainThread ERROR, expected PASS
Traceback (most recent call last):
  File "/Users/jaspreet/src/mozilla-central/testing/marionette/harness/marionette/marionette_test.py", line 344, in run
    testMethod()
  File "/Users/jaspreet/src/mozilla-central/testing/marionette/harness/marionette/marionette_test.py", line 222, in wrapper
    return func(self, *args, **kwargs)
  File "/Users/jaspreet/src/mozilla-central/layout/base/tests/marionette/test_accessiblecaret_selection_mode.py", line 337, in test_long_press_changes_focus_from
    x, y = self.word_location(el2, 0)
  File "/Users/jaspreet/src/mozilla-central/layout/base/tests/marionette/test_accessiblecaret_selection_mode.py", line 102, in word_location
    sel.move_cursor_by_offset(offset)
  File "/Users/jaspreet/src/mozilla-central/testing/marionette/client/marionette_driver/selection.py", line 59, in move_cursor_by_offset
    '''.format(offset, 'backward' if backward else 'forward')
KeyError: '\n                  sel'






4:40.99 TEST_START: MainThread test_standards.py TestStandardsUnits.test_units
 4:41.20 TEST_END: MainThread ERROR, expected PASS
Traceback (most recent call last):
  File "/Users/jaspreet/src/mozilla-central/testing/marionette/harness/marionette/marionette_test.py", line 325, in run
    self.setUp()
  File "/Users/jaspreet/src/mozilla-central/toolkit/components/microformats/test/marionette/test_standards.py", line 13, in setUp
    super(TestStandardsUnits, self).setUp()
  File "/Users/jaspreet/src/mozilla-central/toolkit/components/microformats/test/marionette/../marionette/microformats_tester.py", line 71, in setUp
    "browser.tabs.remote.autostart": True
  File "/Users/jaspreet/src/mozilla-central/testing/marionette/client/marionette_driver/decorators.py", line 42, in _
    return func(*args, **kwargs)
  File "/Users/jaspreet/src/mozilla-central/testing/marionette/client/marionette_driver/marionette.py", line 1023, in enforce_gecko_prefs
    """ .format(pref, value))
KeyError: '\n                case prefInterface'

Comment 12

a year ago
Please ignore,rather delete that attachment.
I updated python files in testing/marionette/client,But the above mentioned files marionette.py and selection.py have code in line 1007 and 56 respectively with which format is creating issues,failing and resulting in several error on running ./mach marionette-test and runs fine if I remove these two specific changes.
(Reporter)

Comment 13

a year ago
Can you please show us the code changes for those problematic lines? Just in the case you want help that would be needed. Thanks.
Flags: needinfo?(dhanvicse)

Comment 14

a year ago
Yes please help..
for marionette.py the line is:
pref_exists = self.execute_script("""
            let prefInterface = Components.classes["@mozilla.org/preferences-service;1"]
                                          .getService(Components.interfaces.nsIPrefBranch);
            let pref = '%s';
            let value = '%s';
            let type = prefInterface.getPrefType(pref);
            switch(type) {
                case prefInterface.PREF_STRING:
                    return value == prefInterface.getCharPref(pref).toString();
                case prefInterface.PREF_BOOL:
                    return value == prefInterface.getBoolPref(pref).toString();
                case prefInterface.PREF_INT:
                    return value == prefInterface.getIntPref(pref).toString();
                case prefInterface.PREF_INVALID:
                    return false;
            }
            """ % (pref, value))
All I change is %s-> {0} %s->{1} and at last %->format
and for selection.py:
cmd = self.js_selection_cmd() + '''
              for (let i = 0; i < %d; ++i) {
                  sel.modify("move", "%s", "character");
              }
              ''' %(offset, 'backward' if backward else 'forward')
Same here...
(Reporter)

Comment 15

a year ago
It's because the extra curly braces we have here. format() tries to retrieve its key but fails. So you will have to double them like "for (..) {{". For details see:

http://stackoverflow.com/questions/9623134/python-format-throws-keyerror
(Reporter)

Updated

a year ago
Keywords: good-first-bug
Whiteboard: [lang=py][good first bug] → [lang=py]

Comment 16

a year ago
I believe the files clients directory are fixed,though i got the result below when testing(as far I remember some tests failed even for original build):
Summary
=======
Ran 739 tests
Expected results: 711
Unexpected results: 2 (ERROR: 1, FAIL: 1)
Skipped: 26
Unexpected Results
==================
FAIL test_screenshot.py Chrome.test_window
ERROR test_key_actions.py TestKeyActions.test_open_in_new_window_shortcut

As in the commit for each directory change is to be made separately.
So what should I be doing to push this for review?
(Reporter)

Comment 17

a year ago
(In reply to jaspreet from comment #16)
> FAIL test_screenshot.py Chrome.test_window

If that is not a syntax error it might be fine. But I cannot say more as long as you do not give the exact failure message as shown in the logs.

> ERROR test_key_actions.py TestKeyActions.test_open_in_new_window_shortcut

This was a known failure which should be fixed on mozilla-central since today. I pushed that fix via bug 1277811.

> As in the commit for each directory change is to be made separately.
> So what should I be doing to push this for review?

When you created all those commits already please setup mozreview by following the steps in this documentation:
https://mozilla-version-control-tools.readthedocs.io/en/latest/mozreview-user.html
(Reporter)

Comment 18

a year ago
Jaspreet, will you have an update for us?
Flags: needinfo?(jaspreet.bits)
Created attachment 8788213 [details]
error-bug-1291687

I am getting the following error when running test of 

testing/marionette/client/marionette_driver/marionette.py
Flags: needinfo?(hskupin)
(Reporter)

Updated

a year ago
Attachment #8788213 - Attachment mime type: application/octet-stream → text/plain
Flags: needinfo?(hskupin)
(Reporter)

Comment 20

a year ago
"testing/marionette/client/marionette_driver/marionette.py" is not a valid test. That's one of the source files for Marionette. What you want is to use the following path to run the tests" testing/marionette/harness/marionette/tests/unit-tests.ini.
(In reply to Henrik Skupin (:whimboo) from comment #20)
> "testing/marionette/client/marionette_driver/marionette.py" is not a valid
> test. That's one of the source files for Marionette. What you want is to use
> the following path to run the tests"
> testing/marionette/harness/marionette/tests/unit-tests.ini.

I assumed that I can pass any file as argument and ./mach marionette-test will run its test, my bad
(Reporter)

Comment 22

a year ago
No, you can only run those files which include tests based on unittest.Testcase. Others won't work because they simply contain no test classes and methods.
Comment hidden (mozreview-request)
(Assignee)

Comment 24

a year ago
Hi, I've started to work on this because I haven't seen any activity since 2016-09-06, hope to have not replicated someone else's work
Please go ahead, Francesco. We'll assign the bug to you once to upload a first patch for review.
(Reporter)

Comment 26

a year ago
Francesco, this is absolutely fantastic work! The amount of changes are massive! It's amazing to see how fast you was able to get this done. I will assign this bug to you now. You have it definitely deserved!

Ok, so before I review the patch I decided to trigger a try build of those changes, so we can see if the linter doesn't recognize issues and that the tests are still passing. The results can be found here:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c15838ae4a8a

When you check that be aware that anything beside green means there are problems. 

Currently I'm traveling for training, and will be on PTO the next week. So if I cannot do the review I'm sure Maja can jump in here and do it instead.
Assignee: nobody → francesco.pischedda
Status: NEW → ASSIGNED
Flags: needinfo?(jaspreet.bits)
(Reporter)

Comment 27

a year ago
mozreview-review
Comment on attachment 8795826 [details]
Bug 1291687 - Ensure string formatting is using `format()` instead of `%` for all Marionette py files:

https://reviewboard.mozilla.org/r/81794/#review80440

The linter job failed and we also have some test failures. Please have a look at the results and get those fixed. In case you have questions how to interprete those, please ask on IRC. Thanks.
Attachment #8795826 - Flags: review?(hskupin)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 31

a year ago
After some fixing I've got to the situation where ./mach marionette-tet has the same result in a clean source tree and mine, there are still some test failing:
Summary
=======

Ran 746 tests
Expected results: 665
Unexpected results: 54 (FAIL: 54)
Skipped: 27

Unexpected Results
==================

this happens both in my source tree and the original one
Comment hidden (mozreview-request)
(Reporter)

Comment 33

a year ago
(In reply to Francesco Pischedda from comment #31)
> After some fixing I've got to the situation where ./mach marionette-tet has
> the same result in a clean source tree and mine, there are still some test
> failing:

Given that lack of information which tests actually are failing, I would assume the reason is that you do not keep Firefox running in the foreground while the tests are active. This is actually a requirement for testing popups. So I assume the failures are caused by this. Please retry running the tests, and do not touch your computer in the meantime.
(Assignee)

Comment 34

a year ago
this is a sample of the failing output
FAIL test_accessiblecaret_selection_mode.py AccessibleCaretSelectionModeTestCase.test_minimum_select_one_character2_content2
FAIL test_accessiblecaret_selection_mode.py AccessibleCaretSelectionModeTestCase.test_minimum_select_one_character2_contenteditable2
FAIL test_accessiblecaret_selection_mode.py AccessibleCaretSelectionModeTestCase.test_minimum_select_one_character2_textarea2
FAIL test_accessiblecaret_selection_mode.py AccessibleCaretSelectionModeTestCase.test_minimum_select_one_character_content
FAIL test_accessiblecaret_selection_mode.py AccessibleCaretSelectionModeTestCase.test_minimum_select_one_character_contenteditable
FAIL test_accessiblecaret_selection_mode.py AccessibleCaretSelectionModeTestCase.test_minimum_select_one_character_input
FAIL test_accessiblecaret_selection_mode.py AccessibleCaretSelectionModeTestCase.test_minimum_select_one_character_textarea
FAIL test_accessiblecaret_selection_mode.py AccessibleCaretSelectionModeTestCase.test_minimum_select_one_character_textarea_rtl

I'll try later without touching the computer to see what happens, thanks for the feedback!
(Assignee)

Comment 35

a year ago
I've tryed every possible way of running the tests but they keep failing in the same way, how can I address this situation?
(Assignee)

Comment 36

a year ago
I've requested commit access level 1 to push to try so I can test everything in a more consistent way (see my failing tests)

can you please vouch me? https://bugzilla.mozilla.org/show_bug.cgi?id=1306575

thanks
Hi Francesco, thanks for taking the time to investigate these tests. I might not have time to get back to you today with a review, but you'll definitely hear from me by Monday.

Comment 38

a year ago
mozreview-review
Comment on attachment 8795826 [details]
Bug 1291687 - Ensure string formatting is using `format()` instead of `%` for all Marionette py files:

https://reviewboard.mozilla.org/r/81794/#review81384

A few small fixes needed.

::: testing/marionette/harness/marionette/runner/base.py:949
(Diff revision 5)
>  
>      def _print_summary(self, tests):
>          self.logger.info('\nSUMMARY\n-------')
> -        self.logger.info('passed: %d' % self.passed)
> +        self.logger.info('passed: {}'.format(self.passed))
>          if self.unexpected_successes == 0:
> -            self.logger.info('failed: %d' % self.failed)
> +            self.logger.info('failed: xs{}%d'.format(self.failed))

xs{}%d

::: testing/marionette/harness/marionette/runner/mixins/browsermob-proxy-py/browsermobproxy/client.py:159
(Diff revision 5)
>  
>          :param domain: domain to set authentication credentials for
>          :param username: valid username to use when authenticating
>          :param  password: valid password to use when authenticating
>          """
> -        r = requests.post(url='%s/proxy/%s/auth/basic/%s' % (self.host, self.port, domain),
> +        r = requests.post(url='{0}/proxy/{1}/auth/basic/%s'.format(self.host, self.port, domain),

leftover `%s`

::: testing/marionette/harness/marionette/tests/unit/test_capabilities.py:107
(Diff revision 5)
>          # same state it was before it started the test
>          self.marionette.start_session()
>  
>      def test_we_get_valid_uuid4_when_creating_a_session(self):
>          self.assertNotIn("{", self.marionette.session_id,
> -                         "Session ID has {} in it: %s" % self.marionette.session_id)
> +                         "Session ID has {} in it: {}".format('{}',

Change this to 'Session ID has {{ in it', since the test is only checking for { anyway.

::: testing/marionette/harness/session/runner/base.py:169
(Diff revision 5)
>          if self.descriptions and doc_first_line:
>              return '\n'.join((str(test), doc_first_line))
>          else:
>              desc = str(test)
>              if hasattr(test, 'jsFile'):
> -                desc = "%s, %s" % (test.jsFile, desc)
> +                desc = "{0} {1}".format(test.jsFile, desc)

Missing `,`

::: testing/marionette/harness/session/runner/base.py:718
(Diff revision 5)
>                  else:
>                      target_tests.append(test)
>  
>              for i in target_tests:
>                  if not os.path.exists(i["path"]):
> -                    raise IOError("test file: %s does not exist" % i["path"])
> +                    raise IOError("test file: {} does not exist".format(["path"]))

missing `i["path"]`

I also pushed to try on your behalf. The new results will be visible in the MozReview page for this patch.
Attachment #8795826 - Flags: review?(mjzffr) → review-
Comment hidden (mozreview-request)
(Assignee)

Comment 40

a year ago
mozreview-review-reply
Comment on attachment 8795826 [details]
Bug 1291687 - Ensure string formatting is using `format()` instead of `%` for all Marionette py files:

https://reviewboard.mozilla.org/r/81794/#review81384

all issues addressed in the latest review request, thanks for reporting

Comment 41

a year ago
mozreview-review
Comment on attachment 8795826 [details]
Bug 1291687 - Ensure string formatting is using `format()` instead of `%` for all Marionette py files:

https://reviewboard.mozilla.org/r/81794/#review81412

r+wc: Please remove the extra lines in your commit message (only keep first line and last line - bug summary and mozreview-commit-id). Once that's done, I will land your patch.
Attachment #8795826 - Flags: review?(mjzffr) → review+
Comment hidden (mozreview-request)
(Assignee)

Comment 43

a year ago
mozreview-review-reply
Comment on attachment 8795826 [details]
Bug 1291687 - Ensure string formatting is using `format()` instead of `%` for all Marionette py files:

https://reviewboard.mozilla.org/r/81794/#review81412

done it
thank you for your help!
(Reporter)

Comment 44

a year ago
Maja, what is the status of this bug? Is there something I should have a look into?
Flags: needinfo?(mjzffr)
Oh?! I missed that the push to autoland failed in mozreview. We need a rebase.
Flags: needinfo?(mjzffr) → needinfo?(francesco.pischedda)
Comment hidden (mozreview-request)
(Assignee)

Comment 47

a year ago
in this review request I have rebased with latest changes from central

Comment 48

a year ago
Pushed by mjzffr@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/cada890e8220
Ensure string formatting is using `format()` instead of `%` for all Marionette py files: r=maja_zf

Comment 49

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/cada890e8220
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
(Assignee)

Comment 50

a year ago
Hi maja_zf and whimboo, thank for your help in solving this issue!

Cheers :)
(Reporter)

Comment 51

a year ago
Francesco, we have to thank you for this amazing work! In case you are interested in something else please let us know. As best on IRC so that we can work on finding something new for you.
(Reporter)

Updated

a year ago
Flags: needinfo?(francesco.pischedda)
(Reporter)

Comment 52

a year ago
Comment on attachment 8795826 [details]
Bug 1291687 - Ensure string formatting is using `format()` instead of `%` for all Marionette py files:

Setting missed r+ from earlier.
Attachment #8795826 - Flags: review?(hskupin) → review+

Comment 53

11 months ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/c2b3d9090a88
status-firefox51: --- → fixed
You need to log in before you can comment on or make changes to this bug.