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

RESOLVED FIXED in Firefox 51

Status

RESOLVED FIXED
3 years ago
2 years 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)

Attachments

(4 attachments)

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}`.
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)
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

3 years 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.
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)
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

3 years ago
On it,I will revert as soon as I finish over that article.

Comment 8

3 years 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.
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

3 years ago
Posted file errors.txt

Comment 11

3 years 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

3 years 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.
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

3 years 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...
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
Keywords: good-first-bug
Whiteboard: [lang=py][good first bug] → [lang=py]

Comment 16

3 years 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?
(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
Jaspreet, will you have an update for us?
Flags: needinfo?(jaspreet.bits)
Posted file error-bug-1291687
I am getting the following error when running test of 

testing/marionette/client/marionette_driver/marionette.py
Flags: needinfo?(hskupin)
Attachment #8788213 - Attachment mime type: application/octet-stream → text/plain
Flags: needinfo?(hskupin)
"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
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

3 years 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.
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

3 years 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

3 years 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)
(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

3 years 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

3 years 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

3 years 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

3 years 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

3 years 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

3 years 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

3 years 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!
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

3 years ago
in this review request I have rebased with latest changes from central

Comment 48

3 years 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

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

Comment 50

3 years ago
Hi maja_zf and whimboo, thank for your help in solving this issue!

Cheers :)
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.
Flags: needinfo?(francesco.pischedda)
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+
You need to log in before you can comment on or make changes to this bug.