Closed Bug 1291687 Opened 8 years ago Closed 8 years ago

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

Categories

(Remote Protocol :: Marionette, defect)

Product:
Remote Protocol
Component:
Marionette
Version:
Version 3
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox51 fixed, firefox52 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla52
Tracking Flags:
Tracking Status
firefox51 --- fixed
firefox52 --- fixed

People

(Reporter: whimboo, Assigned: francesco.pischedda, Mentored)

References

(
URL
)

Details

(Keywords: good-first-bug, Whiteboard: [lang=py])

Attachments

(4 files)

py-files-under-testing-marionette
5.94 KB, text/plain
Details
errors.txt
12.03 KB, text/plain
Details
error-bug-1291687
2.03 KB, text/plain
Details
Bug 1291687 - Ensure string formatting is using `format()` instead of `%` for all Marionette py files:
58 bytes, text/x-review-board-request
whimboo
: review+
impossibus
: review+
Details 
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)
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
URL: http://ateam-bootcamp.readthedocs.io/... → https://wiki.mozilla.org/User:Mjzffr/...
On it,I will revert as soon as I finish over that article.
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.
Attached file errors.txt — 

    
    

    
  
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'

    
    

    
  
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)

    
    

    
  
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]

    
    

    
  
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)

    
    

    
  

      
      
      
      

        Attached 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.

    
    

    
      
  


  

    
    

    
  
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)

    
    

    
  
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)

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
  
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

    
    

    
      
  

    
    

    
  
(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.

    
    

    
  
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!

    
    

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

    
    

    
  
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 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 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 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 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)

    
    

    
      
  

    
    

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

    
    

    
  
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

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/cada890e8220
Status: ASSIGNED → RESOLVED
Closed: 8 years ago

          status-firefox52:
          --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52

    
    

    
  
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+

    
  
Product: Testing → Remote Protocol

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: