1303863 - Marionette: replace all 'restore_pref' and 'restore_all_prefs' with `self.marionette.clear_pref()`

Status: RESOLVED FIXED

(Testing :: Firefox UI Tests, defect)

Description

[Johannes Vogel (:nebelhom)]

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:48.0) Gecko/20100101 Firefox/48.0
Build ID: 20160728204513

Steps to reproduce:

This is the first sub-bug in a line of bugs related to bug #1293588 ( https://bugzilla.mozilla.org/show_bug.cgi?id=1293588 )

Marionette has its own methods for getting and setting preferences. We should make use of those, but right now would have to do some pre-work to make this possible.

Work to do for Marionette before we can switch:

* Fx-ui tests have to be updated to explicitly clear modified preferences at the end of the test via `self.marionette.clear_pref()`, and `restore_pref()`, `restore_all_prefs()` need to be removed.

Comment 1

[Johannes Vogel (:nebelhom)]

:whimboo

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

See my try run above. Everything is green, so no errors as far as I can see...

What is the next step in this workflow? Is there a human proof-reading instance, as well?

Thanks :)

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Not sure what you mean with "human proof-reading instance". But if all is green, its fine. I didn't expect something else when I checked the code earlier.

Regarding the next steps, you should know. The work to be done as listed in comment 0 has not been finished. So I do not see the removal of `restore_pref()` and `restore_all_prefs()`.

Comment 3

[Johannes Vogel (:nebelhom)]

Hi :whimboo ,

I've been looking further into this bug and I have hit a bit of a wall.

Having removed all restore_(all_)prefs() instances (also in the final tear down of the basetestcase), I cannot get the tests to work correctly in test_prefs.py

I cannot create a fitting tearDown for the class using self.marionette.clear_pref or self.prefs.set_pref("to originally assigned value at setUp").

It always causes a 'InvalidArgumentException: Malformed URL: <Value> is not a valid URL.' in every other test that follows after. <Value> can be anything from 54 to 'browser.newtab.url'.

If I comment all code out for testPreferences testcase, then the ./mach firefox-ui-functional runs without trouble.

As I said, I have hit a wall at the moment. Can you give me any suggestions what to try next? Should I better first write a marionette version of restore_all_prefs?

Thanks for any input! :)

Johannes

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+2]]

I don't really understand what you are referring to here. I would propose that you always give at least a subset of the code changes to look at. So please do so that I can help.

We don't need a restore_all_prefs() method for Marionette. Otherwise I would have layed this out on the tracking bug. We have to explicitly reset the modified preferences.

Comment 5

[Johannes Vogel (:nebelhom)]

Ok, I will try to make it clearer. Sorry for that.

Following the below link, you see the base parent for all the tests that I am looking into (as far as I am aware).

https://hg.mozilla.org/mozilla-central/file/c55bcb7c777e/testing/puppeteer/firefox/firefox_puppeteer/testcases/base.py#l112

Once, I remove the highlighted line, all but the first 10 tests in `./mach firefox-ui-functional` fail with the error
'InvalidArgumentException: Malformed URL: 54 is not a valid URL.'

I can track this error back to the file test_prefs.py (please see https://hg.mozilla.org/mozilla-central/file/c55bcb7c777e/testing/firefox-ui/tests/puppeteer/test_prefs.py#l128 )

When I now clear all the prefs using self.marionette.clear_pref by creating a tearDown in this test like shown in this pastebin: https://pastebin.mozilla.org/8913276

The error is the same except: 'InvalidArgumentException: Malformed URL: null is not a valid URL.'

Using any of the two options shown in this pastebin ( https://pastebin.mozilla.org/8913279 ) in order to reset to the default settings gives me

'InvalidArgumentException: Malformed URL: 54 is not a valid URL.'
&
'InvalidArgumentException: Malformed URL: browser.newtab.url is not a valid URL.'
respectively.

I tried various variations of clearing the defined pref in each test, but I am only getting variations of the same error. If I completely comment the file test_prefs.py out, then all the other tests run without any problem.

At this point, I am a little stumped, because I seem to not be able to truly reset to the original settings, using self.marionette.clear_pref() and I am not aware of another way of doing it.

Whimboo, could you please tell this super obvious detail that I am currently missing?

Thanks a lot! :)

Comment 6

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Well, what I actually need are really the code changes you did so far. Upload what you currently have to mozreview or at least attach a full diff to this bug in case mozreview hasn't been setup yet.

Also it is very helpful to get full details of the failure including the stack. Just the failure message isn't that helpful for me. Keep in mind that I could simply import your changes locally if those would be available for download somewhere, and test myself.

thanks

Comment 7

[Johannes Vogel (:nebelhom)]

Attachment: Comment6.diff

Incomplete. Uploaded to identify the issue that blocks me out.

Comment 8

[Johannes Vogel (:nebelhom)]

hi whimboo,

for time constraints, I uploaded a diff. I hope that is helpful. Let me know if it is clear. Otherwise, I'll grab you in IRC.

Thanks :)

Comment 9

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Please also attach the complete failure output or put it in a comment.

Comment 10

[Johannes Vogel (:nebelhom)]

Attachment: gecko-log.txt

error log

Comment 11

[Henrik Skupin [:whimboo][⌚️UTC+2]]

All the problems here are coming from:

  File "/media/nebelhom/Mozilla/src/gecko/testing/puppeteer/firefox/firefox_puppeteer/testcases/base.py", line 106, in setUp
    self.marionette.navigate(self.prefs.get_pref('browser.newtab.url'))

So whatever is stored in 'browser.newtab.url' is not a valid URL. 

Having a look at your code I see the following:

> self.prefs.set_pref(self.string_pref, 'browser.newtab.url')

Whereby `self.string_pref` has `browser.newtab.url` as value. It means you set the preference name as value. 

So whatever you do in a test make sure that this value gets reset before tearDown of the super class gets called. Otherwise this failure will appear. 

It might be wise to change the preference name for the string_pref test to something else less impacting. Just pick another string preference for now. The code will go away anyway soon.

Comment 12

[Johannes Vogel (:nebelhom)]

Attachment: bug1303863.diff

Hi Whimboo,

Here is my first patch for this bug. I attached it because there were some issues with Mozreview that need resolving (essentially git mozreview is not properly set up). I will do that next time, I am online during the week.

The suggested corrections from the try server ( https://treeherder.mozilla.org/#/jobs?repo=try&revision=af1de076f325&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-classifiedState=unclassified&selectedJob=28364454 ) have been incorporated.

Please let me know in case I need to adjust anything.

Thanks :)

Comment 13

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 8796948 [details] [diff] [review]
bug1303863.diff

mozreview would help me a lot to go through this review. I assume you wouldn't mind to get this fixed first before I start the review? Thanks.

Comment 14

[Johannes Vogel (:nebelhom)]

Attachment: Bug 1303863 - Remove restore_pref and restore_all_prefs methods from Firefox Puppeteer.

Review commit: https://reviewboard.mozilla.org/r/84298/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/84298/

Comment 15

[Henrik Skupin [:whimboo][⌚️UTC+2]]

So, is this patch ready for review or not? If yes, please set me as reviewer in mozreview. Thanks

Comment 16

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 8798986 [details]
Bug 1303863 - Remove restore_pref and restore_all_prefs methods from Firefox Puppeteer.

https://reviewboard.mozilla.org/r/84298/#review83252

Thank you for the update Johannes. A quick skim over the files showed me that you didn't remove the restore related methods from the actual prefs.py module of puppeteer. Please do so, and when you pushed an update please also trigger a try build. That way I can see if all is fine.

::: testing/firefox-ui/tests/functional/security/test_safe_browsing_warning_pages.py:43
(Diff revision 1)
>      def tearDown(self):
>          try:
>              self.utils.permissions.remove('https://www.itisatrap.org', 'safe-browsing')
>              self.browser.tabbar.close_all_tabs([self.browser.tabbar.tabs[0]])
> +            self.marionette.clear_pref('browser.safebrowsing.phishing.enabled')
> +            self.marionette.clear_pref('browser.safebrowsing.malware.enabled')

Please try to keep the alphabetical order as best as possible.

::: testing/firefox-ui/tests/puppeteer/test_notifications.py:32
(Diff revision 1)
>              self.utils.permissions.remove(self.addons_url, 'install')
>  
>              if self.browser.notification:
>                  self.browser.notification.close(force=True)
> +
> +            self.marionette.clear_pref('extensions.install.requireSecureOrigin')

Lets make sure that we reset preferences as first in the tearDown. If we miss one because a line above raises an exception following tests will be busted. I feel it more critical as closing eg a browser notification.

Also what about `xpinstall.signatures.required` which is used later in this test?

::: testing/firefox-ui/tests/puppeteer/test_prefs.py:18
(Diff revision 1)
> -        self.assertTrue(self.prefs.reset_pref(self.new_pref))
> -        self.assertEqual(self.prefs.get_pref(self.new_pref), None)
>  
> -        # There is no such preference anymore
> -        self.assertFalse(self.prefs.reset_pref(self.new_pref))
> +        self.bool_pref = 'browser.tabs.loadBookmarksInBackground'  # default is false
> +        self.int_pref = 'browser.tabs.maxOpenBeforeWarn'  # default is 15
> +        self.string_pref = 'browser.newtab.url'  # default is about:newtab

I don't think that we should add the default comments at the end of the lines. Those settings specify the name of preferences but not their (default) values.

::: testing/firefox-ui/tests/puppeteer/test_prefs.py:48
(Diff revision 1)
>          value = self.prefs.get_pref('browser.startup.homepage',
>                                      interface='nsIPrefLocalizedString')
>          self.assertNotEqual(value, properties_file)
>  
> -    def test_restore_pref(self):
> -        # test with single set_pref call and a new preference
> +        # reset values to original
> +        self.prefs.set_pref(self.int_pref, '15')

This line doesn't reset the pref but simply sets another value. Please add a call to `clear_pref()` in the `tearDown` method.

::: testing/firefox-ui/tests/puppeteer/test_prefs.py:79
(Diff revision 1)
>          self.assertEqual(self.prefs.get_pref(self.string_pref), '54')
>  
> +        # reset values to original
> +        self.prefs.set_pref(self.bool_pref, 'false')
> +        self.prefs.set_pref(self.int_pref, '15')
> +        self.prefs.set_pref(self.string_pref, 'about:newtab')

Same here and for the following. It doesn't reset the prefs but simply adds another value.

::: testing/firefox-ui/tests/puppeteer/test_prefs.py:91
(Diff revision 1)
> +        self.prefs.set_pref(self.bool_pref, 'false')
> +
>      def test_set_pref_new_preference(self):
>          self.prefs.set_pref(self.new_pref, True)
>          self.assertTrue(self.prefs.get_pref(self.new_pref))
> -        self.prefs.restore_pref(self.new_pref)
> +        # clear value

No need for those comments.

Comment 17

[Johannes Vogel (:nebelhom)]

Comment on attachment 8798986 [details]
Bug 1303863 - Remove restore_pref and restore_all_prefs methods from Firefox Puppeteer.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/84298/diff/1-2/

Comment 18

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 8798986 [details]
Bug 1303863 - Remove restore_pref and restore_all_prefs methods from Firefox Puppeteer.

https://reviewboard.mozilla.org/r/84298/#review84040

As Johannes told me yesterday he didn't fix all the mentioned issues. So removing review flag for now.

Comment 19

[Johannes Vogel (:nebelhom)]

Comment on attachment 8798986 [details]
Bug 1303863 - Remove restore_pref and restore_all_prefs methods from Firefox Puppeteer.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/84298/diff/2-3/

Comment 20

[Johannes Vogel (:nebelhom)]

Hi whimboo,

I triggered another review round although I still have one failure in the test run.

Could you please have a look at it, once the try server has run? I cannot figure out why the failure occurs.

Thanks a lot for your help :)

Comment 21

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 8798986 [details]
Bug 1303863 - Remove restore_pref and restore_all_prefs methods from Firefox Puppeteer.

https://reviewboard.mozilla.org/r/84298/#review85436

::: testing/firefox-ui/tests/puppeteer/test_prefs.py:92
(Diff revision 3)
>          self.prefs.set_pref(self.new_pref, True)
>          self.assertTrue(self.prefs.get_pref(self.new_pref))
> -        self.prefs.restore_pref(self.new_pref)
>  
>          self.prefs.set_pref(self.new_pref, 5)
>          self.assertEqual(self.prefs.get_pref(self.new_pref), 5)

This assertion is raising the failure for the e10s tests: https://treeherder.mozilla.org/logviewer.html#?job_id=29198598&repo=try#L5088

So you are trying to change the type of a preference from Boolean to Int. This will not work. To fix that you really have to reset the preference for each block in that method.

::: testing/puppeteer/firefox/firefox_puppeteer/api/prefs.py:5
(Diff revision 3)
>  # This Source Code Form is subject to the terms of the Mozilla Public
>  # License, v. 2.0. If a copy of the MPL was not distributed with this file,
>  # You can obtain one at http://mozilla.org/MPL/2.0/.
>  
>  from marionette_driver.errors import MarionetteException

Please fix the f8 lint warning:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f12ba3b9402d&selectedJob=29197452

Comment 22

[Johannes Vogel (:nebelhom)]

Comment on attachment 8798986 [details]
Bug 1303863 - Remove restore_pref and restore_all_prefs methods from Firefox Puppeteer.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/84298/diff/3-4/

Comment 23

[Johannes Vogel (:nebelhom)]

Comment on attachment 8798986 [details]
Bug 1303863 - Remove restore_pref and restore_all_prefs methods from Firefox Puppeteer.

https://reviewboard.mozilla.org/r/84298/#review85436

> Please fix the f8 lint warning:
> 
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=f12ba3b9402d&selectedJob=29197452

Hi Whimboo, I am not sure what it is. When I click on it, I receive all green for it... What is the issue exactly? Thanks

Comment 24

[Johannes Vogel (:nebelhom)]

Hi whimboo,

quick update for you.

I fixed the test_prefs.py issue. All that remains is the f8 lint warning that I cannot see for some reason.

Once that is fixed, as well, I think we can land this bug... finally :)

Comment 25

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 8798986 [details]
Bug 1303863 - Remove restore_pref and restore_all_prefs methods from Firefox Puppeteer.

https://reviewboard.mozilla.org/r/84298/#review85436

> Hi Whimboo, I am not sure what it is. When I click on it, I receive all green for it... What is the issue exactly? Thanks

Strange, where did this test result went? I cannot see it anymore on Treeherder. Can you please do me a favor and run the following locally? The command is "mach lint" and will lint everything. If you only want our folders specify it as extra argument.

Otherwise let me create another try build and we might see results there.

Comment 26

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 8798986 [details]
Bug 1303863 - Remove restore_pref and restore_all_prefs methods from Firefox Puppeteer.

https://reviewboard.mozilla.org/r/84298/#review86936

::: testing/firefox-ui/tests/puppeteer/test_prefs.py:62
(Diff revisions 3 - 4)
>      def test_set_pref_casted_values(self):
>          # basestring as boolean
>          self.prefs.set_pref(self.bool_pref, '')
>          self.assertFalse(self.prefs.get_pref(self.bool_pref))
> -
> +        self.marionette.clear_pref(self.bool_pref)
> +        

nit: left-over blanks which will definitely cause a lint failure.

::: testing/firefox-ui/tests/puppeteer/test_prefs.py:57
(Diff revision 4)
> -        self.prefs.restore_all_prefs()
> -        self.assertEqual(self.prefs.get_pref(self.bool_pref), orig_bool)
> -        self.assertEqual(self.prefs.get_pref(self.int_pref), orig_int)
> -        self.assertEqual(self.prefs.get_pref(self.string_pref), orig_string)
> -
>      def test_set_pref_casted_values(self):

While thinking more about this test method it looks like that setting the pref via marionette doesn't allow us to cast values anymore. This is fine and should make the tests more robust in terms of inappropriate preference value changes.

Can you please check that? If it is really the case we should better remove this test, or update it that we expect those exceptions. Sorry, that I didn't notice that before.

Comment 27

[Johannes Vogel (:nebelhom)]

hi whimboo,

I am not 100% sure I understand what you mean by testing if I can cast values.

The entire trouble with test_prefs.py arose because `self.prefs.set_pref` cannot be called twice on the same pref without calling `self.marionette.clear_pref` in between the two calls.

Is this what you meant? If not, could you please spell it out for me?

Thanks and sorry for being thick-headed :)

P.S. I found the lint error. There was some whitespace in test_pref.py

Comment 28

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to Johannes Vogel (:nebelhom) from comment #27)
> I am not 100% sure I understand what you mean by testing if I can cast
> values.

As it looks like we cannot set a Number value for a String preferences. As result an exception gets raised. We should update the test so we originally set a Boolean pref, and then try to set a Number and String value. You can assert for exceptions via `self.assertRaises()` - you will find a number of those lines in the existent tests.

> The entire trouble with test_prefs.py arose because `self.prefs.set_pref`
> cannot be called twice on the same pref without calling
> `self.marionette.clear_pref` in between the two calls.

It can be called twice, but not when you change the type of preference value.

Comment 29

[Johannes Vogel (:nebelhom)]

Hi whimboo,

I will try to catch you in IRC on this. I'd rather chat with you about this in detail before I make any changes and we end up doing 3 more mozreview cycles...

Speak to you soon :)

Comment 30

[Johannes Vogel (:nebelhom)]

Comment on attachment 8798986 [details]
Bug 1303863 - Remove restore_pref and restore_all_prefs methods from Firefox Puppeteer.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/84298/diff/4-5/

Comment 31

[Johannes Vogel (:nebelhom)]

Comment on attachment 8798986 [details]
Bug 1303863 - Remove restore_pref and restore_all_prefs methods from Firefox Puppeteer.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/84298/diff/5-6/

Comment 32

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 8798986 [details]
Bug 1303863 - Remove restore_pref and restore_all_prefs methods from Firefox Puppeteer.

https://reviewboard.mozilla.org/r/84298/#review89572

There are only comments left which need an update. Code-wise it seems to be fine. I will trigger a try build for safety, while you can update the patch. Once both are done we can get those changes landed. Thanks!

::: testing/firefox-ui/tests/puppeteer/test_prefs.py:19
(Diff revisions 4 - 6)
>          self.unknown_pref = 'marionette.unittest.unknown'
>  
>          self.bool_pref = 'browser.tabs.loadBookmarksInBackground'
>          self.int_pref = 'browser.tabs.maxOpenBeforeWarn'
> +        # Consider changing back to "browser.newtab.url"
> +        # when marionette fully implemented

No. We do not want to use the `browser.newtab.url` anymore given that this can have bad side-effects for us. I'm ok with `browser.startup.homepage` for now, but in one of the next patches for bug 1293588 we should use new test preferences instead.

::: testing/firefox-ui/tests/puppeteer/test_prefs.py:63
(Diff revisions 4 - 6)
>  
>      def test_set_pref_casted_values(self):
>          # basestring as boolean
>          self.prefs.set_pref(self.bool_pref, '')
>          self.assertFalse(self.prefs.get_pref(self.bool_pref))
> +        # Remove when all self.marionette methods are implemented

Please add the reference to bug 1293588 here, like `# Bug 1293588: comment`.

Comment 33

[Johannes Vogel (:nebelhom)]

Comment on attachment 8798986 [details]
Bug 1303863 - Remove restore_pref and restore_all_prefs methods from Firefox Puppeteer.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/84298/diff/6-7/

Comment 34

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 8798986 [details]
Bug 1303863 - Remove restore_pref and restore_all_prefs methods from Firefox Puppeteer.

https://reviewboard.mozilla.org/r/84298/#review89672

Sorry for one more thing but when I tried to land I noticed that I actually want a better commit message. Please change it to "Bug 1303863 - Remove restore_pref and restore_all_prefs methods from Firefox Puppeteer."

Comment 35

[Johannes Vogel (:nebelhom)]

Comment on attachment 8798986 [details]
Bug 1303863 - Remove restore_pref and restore_all_prefs methods from Firefox Puppeteer.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/84298/diff/7-8/

Comment 36

[Johannes Vogel (:nebelhom)]

Comment on attachment 8798986 [details]
Bug 1303863 - Remove restore_pref and restore_all_prefs methods from Firefox Puppeteer.

https://reviewboard.mozilla.org/r/84298/#review89672

Done. Pushing it now to mozreview

Comment 37

[Johannes Vogel (:nebelhom)]

Comment on attachment 8798986 [details]
Bug 1303863 - Remove restore_pref and restore_all_prefs methods from Firefox Puppeteer.

https://reviewboard.mozilla.org/r/84298/#review89572

I have updated them now as discussed and pushed to mozreview again.

Thanks a lot, whimboo :)

Comment 38

[Johannes Vogel (:nebelhom)]

Comment on attachment 8798986 [details]
Bug 1303863 - Remove restore_pref and restore_all_prefs methods from Firefox Puppeteer.

https://reviewboard.mozilla.org/r/84298/#review86936

> nit: left-over blanks which will definitely cause a lint failure.

fixed. no more errors in lint. There was some whitespace.

> While thinking more about this test method it looks like that setting the pref via marionette doesn't allow us to cast values anymore. This is fine and should make the tests more robust in terms of inappropriate preference value changes.
> 
> Can you please check that? If it is really the case we should better remove this test, or update it that we expect those exceptions. Sorry, that I didn't notice that before.

As discussed on IRC, we markup that self.clear_pref() needs removing once the entire test was migrated to self.marionette methods

Comment 39

[Johannes Vogel (:nebelhom)]

Comment on attachment 8798986 [details]
Bug 1303863 - Remove restore_pref and restore_all_prefs methods from Firefox Puppeteer.

https://reviewboard.mozilla.org/r/84298/#review85436

> Strange, where did this test result went? I cannot see it anymore on Treeherder. Can you please do me a favor and run the following locally? The command is "mach lint" and will lint everything. If you only want our folders specify it as extra argument.
> 
> Otherwise let me create another try build and we might see results there.

This is addressed now and I have no lint errors anymore

Comment 40

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/19dc782bc2bd
Remove restore_pref and restore_all_prefs methods from Firefox Puppeteer. r=whimboo

Comment 41

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/19dc782bc2bd