Closed
Bug 1432894
Opened 7 years ago
Closed 7 years ago
Remove deprecated Marionette preferences
Categories
(Remote Protocol :: Marionette, enhancement, P3)
Tracking
(firefox61 fixed)
RESOLVED
FIXED
mozilla61
Tracking | Status | |
---|---|---|
firefox61 | --- | fixed |
People
(Reporter: ato, Assigned: ato)
References
Details
Attachments
(4 files)
Marionette has quite a complicated system in
testing/marionette/components/marionette.js for support fallback
preferences for earlier Firefoxen. Since Firefox 55 shipped
quite some time ago it is time to remove the fallbacks. This will
significantly simplify the overall complexity of this file.
Assignee | ||
Updated•7 years ago
|
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → ato
Comment 1•7 years ago
|
||
I actually would do that after the next ESR release, which would be in the 61 train.
Assignee | ||
Comment 2•7 years ago
|
||
I don’t think we have to wait that long. We left the fallbacks
in place, not for the Firefox update tests, but for geckodriver.
The most recent version of geckodriver supports Firefox 55 and
greater and uses the correct prefs.
Assignee | ||
Comment 3•7 years ago
|
||
Oh crap, we forgot to update geckoinstance.py [1], which means we need
to fix the Python client and release that before we can make this change:
> level = "TRACE" if self.verbose >= 2 else "DEBUG"
> profile_args["preferences"]["marionette.logging"] = level
[1] https://searchfox.org/mozilla-central/rev/062e1cf551f5bf3f0af33671b818f75a55ac497b/testing/marionette/client/marionette_driver/geckoinstance.py#178
Assignee | ||
Updated•7 years ago
|
Summary: Remove fallback preferences → Remove deprecated Marionette preferences
Updated•7 years ago
|
Updated•7 years ago
|
Assignee | ||
Updated•7 years ago
|
Priority: -- → P3
Updated•7 years ago
|
Assignee: ato → nobody
Comment 4•7 years ago
|
||
Andreas, do we want to make it a mentored bug, or will you just fix it yourself now that it is unblocked?
Flags: needinfo?(ato)
Assignee | ||
Comment 5•7 years ago
|
||
I don’t care either way. Can’t tell you when I will fix it though.
Flags: needinfo?(ato)
Assignee | ||
Comment 6•7 years ago
|
||
Looking a bit closer at the code, it is probably best if I dedicate
some attention to this.
Assignee: nobody → ato
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 10•7 years ago
|
||
mozreview-review |
Comment on attachment 8967971 [details]
Bug 1432894 - Provide shorthands for Marionette prefs.
https://reviewboard.mozilla.org/r/236660/#review242428
::: testing/marionette/prefs.js:245
(Diff revision 1)
> +class EnvironmentPrefs {
> + static* from(key) {
> + if (!env.exists(key)) {
> + throw new TypeError("Environment variable not set: " + key);
> + }
> +
> + let prefs;
> + try {
> + prefs = JSON.parse(env.get(key));
> + } catch (e) {
> + throw new TypeError(`Unable to parse prefs from ${key}`, e);
> + }
> +
> + for (let prefName of Object.keys(prefs)) {
> + yield [prefName, prefs[prefName]];
> + }
> + }
> +}
Docs.
Assignee | ||
Comment 11•7 years ago
|
||
mozreview-review |
Comment on attachment 8967970 [details]
Bug 1432894 - Rely on MarionetteMainProcess#enabled instead of pref.
https://reviewboard.mozilla.org/r/236658/#review242430
::: commit-message-37b88:3
(Diff revision 1)
> +Bug 1432894 - Rely on MarionetteMainProcess#enabled instead of pref. r?whimboo
> +
> +The MarionetteMainProcess#enabled getter returns the value of the preference so it is more appropriate to use the correct API than to fetch the preference directly.
fmt
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Status: NEW → ASSIGNED
Comment 16•7 years ago
|
||
mozreview-review |
Comment on attachment 8968125 [details]
Bug 1432894 - Register marionette.contentListener pref.
https://reviewboard.mozilla.org/r/236800/#review242632
Attachment #8968125 -
Flags: review?(hskupin) → review+
Comment 17•7 years ago
|
||
mozreview-review |
Comment on attachment 8967970 [details]
Bug 1432894 - Rely on MarionetteMainProcess#enabled instead of pref.
https://reviewboard.mozilla.org/r/236658/#review242634
Attachment #8967970 -
Flags: review?(hskupin) → review+
Comment 18•7 years ago
|
||
mozreview-review |
Comment on attachment 8967971 [details]
Bug 1432894 - Provide shorthands for Marionette prefs.
https://reviewboard.mozilla.org/r/236660/#review242638
::: commit-message-7bede:11
(Diff revision 2)
> +to mutate at runtime.
> +
> +The new module additionally provides a preference abstraction on
> +top of nsIPrefService instead of Preferences.jsm. We cannot use
> +Preferences.jsm during startup in Marionette because Marionette
> +gets loaded unconditionally.
Can you please explain this a bit further? What exactly blocks us from using Preferences.jsm?
::: testing/marionette/prefs.js:7
(Diff revision 2)
> + * 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/. */
> +
> +"use strict";
> +
> +ChromeUtils.import("resource://gre/modules/Log.jsm");
I don't see that this is used in this module?
::: testing/marionette/prefs.js:33
(Diff revision 2)
> + *
> + * const {Prefs} = ChromeUtils.import("chrome://marionette/content/prefs.js", {});
> + * Prefs.set("marionette.port", 777);
> + * let port = Prefs.get("marionette.port");
> + */
> +class Prefs {
Why that extra abstraction around the Branch class?
::: testing/marionette/prefs.js:38
(Diff revision 2)
> +class Prefs {
> + /**
> + * Gets value of `pref` in its known type.
> + *
> + * @param {string} pref
> + * Preference key.
"Name of `pref`" please.
::: testing/marionette/prefs.js:44
(Diff revision 2)
> + *
> + * @return {(string|boolean|number)}
> + * Value of `pref`.
> + *
> + * @throws {TypeError}
> + * If `pref` is not a preference.
You mean existent preference?
::: testing/marionette/prefs.js:54
(Diff revision 2)
> +
> + /**
> + * Sets the value of `pref`.
> + *
> + * @param {string} pref
> + * Preference key.
Same as above.
::: testing/marionette/prefs.js:56
(Diff revision 2)
> + * Sets the value of `pref`.
> + *
> + * @param {string} pref
> + * Preference key.
> + * @param {(string|boolean|number)} value
> + * Prefernece `pref`'s new value.
Check spelling please
::: testing/marionette/prefs.js:100
(Diff revision 2)
> + case PREF_INT:
> + return this._branch.getIntPref(pref);
> +
> + case PREF_INVALID:
> + default:
> + throw new TypeError(`Unrecognised preference: ${pref}`);
Lets say: "Unrecognised type for preference: ..."
::: testing/marionette/prefs.js:170
(Diff revision 2)
> + */
> +class MarionetteBranch extends Branch {
> + constructor(branch = "marionette.") {
> + super(branch);
> + this.log = new LogBranch(branch + "log.");
> + this.prefs = new PrefsBranch(branch + "prefs.");
I wouldn't make it that complicated. Having just the MarionetteBranch is fine, but doing the same for each and every sub-branch is overhead it will make it harder to search for usages of specific preferences. Also those other classes have a single method only.
::: testing/marionette/prefs.js:178
(Diff revision 2)
> + /**
> + * Gets the `marionette.enabled` preference.
> + *
> + * @return {boolean}
> + */
> + get enabled() {
This causes a test failure on Android, where "marionette.enabled" is not recognized. Maybe it's also causing all the other retry failures, which cause no single job on Android to pass.
::: testing/marionette/prefs.js:243
(Diff revision 2)
> + }
> + }
> +}
> +
> +class PrefsBranch extends Branch {
> + constructor(branch = "marionette.prefs.") {
Just a side question here, why do we actually carry this "pref" branch around, and not use "marionette.recommended.enabled", which is more self-explaining?
::: testing/marionette/test/unit/test_prefs.js:7
(Diff revision 2)
> + * 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/. */
> +
> +"use strict";
> +
> +ChromeUtils.import("resource://gre/modules/Log.jsm");
Unused here too.
::: testing/marionette/test/unit/test_prefs.js:114
(Diff revision 2)
> + let prefsTable = {
> + "marionette.enabled": true,
> + "marionette.port": 777,
> + "marionette.log.level": "foo",
> + };
> + env.set("FOO", JSON.stringify(prefsTable));
Please reset this too so that it is not carried around.
Attachment #8967971 -
Flags: review?(hskupin) → review-
Comment 19•7 years ago
|
||
mozreview-review |
Comment on attachment 8967972 [details]
Bug 1432894 - Remove fallback preferences from Marionette.
https://reviewboard.mozilla.org/r/236662/#review242652
::: testing/marionette/components/marionette.js:20
(Diff revision 2)
> + EnvironmentPrefs,
> + MarionettePrefs,
> + Prefs,
> +} = ChromeUtils.import("chrome://marionette/content/prefs.js", {});
> ChromeUtils.defineModuleGetter(this, "Preferences",
> "resource://gre/modules/Preferences.jsm");
We should get rid of this usage of Preference.jsm too.
::: testing/marionette/components/marionette.js
(Diff revision 2)
> -
> - case PREF_INVALID:
> - return undefined;
> -
> - default:
> - throw new TypeError(`Unexpected preference type (${type}) for ${pref}`);
Actually this would have been a better failure message for the other commit.
::: testing/marionette/components/marionette.js
(Diff revision 2)
> - for (let prefName of Object.keys(prefs)) {
> - Preferences.set(prefName, prefs[prefName]);
> - }
> - }
> - }
> - },
It would be better, if you would have moved this code to `prefs.js` first. It's mainly identical, and would have helped me a lot when reviewing it.
::: testing/marionette/components/marionette.js:439
(Diff revision 2)
> for (let [k, v] of RECOMMENDED_PREFS) {
> if (!Preferences.isSet(k)) {
> log.debug(`Setting recommended pref ${k} to ${v}`);
> Preferences.set(k, v);
> this.alteredPrefs.add(k);
> }
Maybe something for a follow-up, or another commit on this patch series... please add a try/catch around each `Preferences.set()` call. Otherwise if we fail to set a single preference, we won't set any other following.
Attachment #8967972 -
Flags: review?(hskupin)
Assignee | ||
Comment 20•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8967971 [details]
Bug 1432894 - Provide shorthands for Marionette prefs.
https://reviewboard.mozilla.org/r/236660/#review242638
> Can you please explain this a bit further? What exactly blocks us from using Preferences.jsm?
See https://bugzilla.mozilla.org/show_bug.cgi?id=1357517#c0.
Toolkit peers say it “adds overhead” and would like to remove it
altogether in favour of nsIPrefService.
> I don't see that this is used in this module?
It is used in testing/marionette/prefs.js:221 (the LogBranch#level
getter) for deserialising the likes of "info" (string) to Log.Level.Info.
> Why that extra abstraction around the Branch class?
This returns the root level preference tree and exposes static
functions that lets you interact with any preference much like how
Preferences.jsm work:
> Prefs.set("marionette.enabled", true);
> Lets say: "Unrecognised type for preference: ..."
We already know the type: it can only be a string, boolean, or integer.
This branch gets called when the preference doesn’t exist.
> I wouldn't make it that complicated. Having just the MarionetteBranch is fine, but doing the same for each and every sub-branch is overhead it will make it harder to search for usages of specific preferences. Also those other classes have a single method only.
What would you propose? A flat API like MarionettePrefs.logLevel
instead of MarionettePrefs.log.level?
> Just a side question here, why do we actually carry this "pref" branch around, and not use "marionette.recommended.enabled", which is more self-explaining?
marionette.recommended.enabled does not exist.
> Unused here too.
Used in test_MarionettePrefs_getters on
testing/marionette/test/unit/test_prefs.js:126.
Assignee | ||
Comment 21•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8967972 [details]
Bug 1432894 - Remove fallback preferences from Marionette.
https://reviewboard.mozilla.org/r/236662/#review242652
> We should get rid of this usage of Preference.jsm too.
That requires some more work, e.g. for Preferences.isSet(), so I’d
like to do a follow-up on that when we’ve clarified the future of
that module.
> Actually this would have been a better failure message for the other commit.
Actually this error message is wrong. When getting a preference
it is always one of the three PREF_STRING, PREF_INT, or PREF_BOOL
branches. The default branch will only be hit when the preference
does not exist.
> Maybe something for a follow-up, or another commit on this patch series... please add a try/catch around each `Preferences.set()` call. Otherwise if we fail to set a single preference, we won't set any other following.
Isn’t that expected behaviour? Should we gloss over a real issue
when passed the wrong preference type?
As far as I know this will only happen if we make a mistake in
encoding a recommended preference value type, i.e. it cannot happen
with user input. In this case we should arguably fail hard.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 26•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8967971 [details]
Bug 1432894 - Provide shorthands for Marionette prefs.
https://reviewboard.mozilla.org/r/236660/#review242638
> See https://bugzilla.mozilla.org/show_bug.cgi?id=1357517#c0.
>
> Toolkit peers say it “adds overhead” and would like to remove it
> altogether in favour of nsIPrefService.
Right, I noticed that when further reviewing the patch but actually missed to remove this issue. It's all clear now.
> It is used in testing/marionette/prefs.js:221 (the LogBranch#level
> getter) for deserialising the likes of "info" (string) to Log.Level.Info.
Oh, kinda hidden which I didn't notice. Thanks.
> This returns the root level preference tree and exposes static
> functions that lets you interact with any preference much like how
> Preferences.jsm work:
>
> > Prefs.set("marionette.enabled", true);
Right, but is that needed, at least yet? I mean if we finally get rid of `Preferences.jsm` (see the other commit), it could be useful.
> We already know the type: it can only be a string, boolean, or integer.
>
> This branch gets called when the preference doesn’t exist.
No, this is the switch statement based on the result of `getPrefType()`. Keep in mind that a preference can also be complex like `nsIFile`, or `nsIPrefLocalizedString`. For those we would report a wrong failure.
> What would you propose? A flat API like MarionettePrefs.logLevel
> instead of MarionettePrefs.log.level?
Currently yes.
> marionette.recommended.enabled does not exist.
Correct, that's why I'm asking. Because `marionette.prefs.recommended` is not that clear. Maybe `marionette.set_recommended_prefs` would be better. But not sure that we should do that hassle, which would need fallback code again.
> Used in test_MarionettePrefs_getters on
> testing/marionette/test/unit/test_prefs.js:126.
Correct.
Comment 27•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8967972 [details]
Bug 1432894 - Remove fallback preferences from Marionette.
https://reviewboard.mozilla.org/r/236662/#review242652
> That requires some more work, e.g. for Preferences.isSet(), so I’d
> like to do a follow-up on that when we’ve clarified the future of
> that module.
Ok, so please file a bug and close this issue. Thanks.
> Actually this error message is wrong. When getting a preference
> it is always one of the three PREF_STRING, PREF_INT, or PREF_BOOL
> branches. The default branch will only be hit when the preference
> does not exist.
No, you missed complex as said in the other review.
> Isn’t that expected behaviour? Should we gloss over a real issue
> when passed the wrong preference type?
>
> As far as I know this will only happen if we make a mistake in
> encoding a recommended preference value type, i.e. it cannot happen
> with user input. In this case we should arguably fail hard.
Thinking more about this it makes indeed sense to fail hard here.
Assignee | ||
Comment 28•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8967971 [details]
Bug 1432894 - Provide shorthands for Marionette prefs.
https://reviewboard.mozilla.org/r/236660/#review242638
> Right, but is that needed, at least yet? I mean if we finally get rid of `Preferences.jsm` (see the other commit), it could be useful.
I guess you are right: no need to make this unnecessarily complicated.
I’ve dropped the Prefs class for now.
> No, this is the switch statement based on the result of `getPrefType()`. Keep in mind that a preference can also be complex like `nsIFile`, or `nsIPrefLocalizedString`. For those we would report a wrong failure.
‘No’ PREF_INVALID doesn’t get called if the preference doesn’t
exist, or ‘no’ the error message is still wrong?
You are right about PREF_COMPLEX and I will amend it to handle that
case. As I understand it the message will be correct once that is
covered?
> Currently yes.
That works too.
> Correct, that's why I'm asking. Because `marionette.prefs.recommended` is not that clear. Maybe `marionette.set_recommended_prefs` would be better. But not sure that we should do that hassle, which would need fallback code again.
Well it’s a valid question… I guess I don’t find
marionette.prefs.recommended that confusing, but I can see the
counter argument to that. If you feel deeply about this we can
file a bug but for the time being the latest amendment to this patch
makes getting the preference value look like Marionette.recommendedPrefs,
which should be easier to read.
Assignee | ||
Comment 29•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8967972 [details]
Bug 1432894 - Remove fallback preferences from Marionette.
https://reviewboard.mozilla.org/r/236662/#review242652
> Ok, so please file a bug and close this issue. Thanks.
Since I dropped Prefs, we will keep Preferences around here and the
above isSet concern is not valid.
> No, you missed complex as said in the other review.
I’ve addressed PREF_COMPLEX in the other patch, so we can continue
the discussion on that issue.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 41•7 years ago
|
||
Finally the try run appears green:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0c2b9df73e9c68a05e9d8007f34215af64a63a2b
It paid off to allow Branch#get to take a fallback value. Of course
in the longer term we also need a fix for installing the missing
preferences on Fennec. For this I’ve filed
https://bugzilla.mozilla.org/show_bug.cgi?id=1454876.
Comment 42•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8967971 [details]
Bug 1432894 - Provide shorthands for Marionette prefs.
https://reviewboard.mozilla.org/r/236660/#review242638
> ‘No’ PREF_INVALID doesn’t get called if the preference doesn’t
> exist, or ‘no’ the error message is still wrong?
>
> You are right about PREF_COMPLEX and I will amend it to handle that
> case. As I understand it the message will be correct once that is
> covered?
Yes, that should be fine now.
Comment 43•7 years ago
|
||
mozreview-review |
Comment on attachment 8967971 [details]
Bug 1432894 - Provide shorthands for Marionette prefs.
https://reviewboard.mozilla.org/r/236660/#review243482
::: testing/marionette/prefs.js:80
(Diff revision 6)
> +
> + /**
> + * Sets the value of `pref`.
> + *
> + * @param {string} pref
> + * Preference key.
s/key/name as above. Check other instances too please.
::: testing/marionette/prefs.js:183
(Diff revision 6)
> +
> + /**
> + * Fail-safe return of the current log level from preference
> + * `marionette.log.level`.
> + *
> + * @return {Log.LEvel}
`Log.Level`
::: testing/marionette/prefs.js:205
(Diff revision 6)
> + default:
> + return Log.Level.Info;
> + }
> + }
> +
> + get recommendedPrefs() {
Please add a JSdoc here.
::: testing/marionette/test/unit/test_prefs.js:108
(Diff revision 6)
> +
> +add_test(function test_MarionettePrefs_getters() {
> + equal(false, MarionettePrefs.enabled);
> + equal(2828, MarionettePrefs.port);
> + equal(Log.Level.Info, MarionettePrefs.logLevel);
> + equal(true, MarionettePrefs.recommendedPrefs);
Maybe we should only test if those exist, but not their value. Otherwise if defaults change (like in a different application) the test would also have to be updated.
::: testing/marionette/test/unit/test_prefs.js:118
(Diff revision 6)
> +add_test(function test_MarionettePrefs_setters() {
> + try {
> + MarionettePrefs.enabled = true;
> + MarionettePrefs.port = 777;
> + equal(true, MarionettePrefs.enabled);
> + equal(777, MarionettePrefs.port);
Shouldn't it be enough to test a single pref? Even maybe a less critical one as the enabled state or port? What about the log level?
::: testing/marionette/test/unit/test_prefs.js:120
(Diff revision 6)
> + MarionettePrefs.enabled = true;
> + MarionettePrefs.port = 777;
> + equal(true, MarionettePrefs.enabled);
> + equal(777, MarionettePrefs.port);
> + } finally {
> + reset();
`reset()` doesn't reset the Marionette but the test preferences. You will leave behind modified Marionette preferences.
Attachment #8967971 -
Flags: review?(hskupin) → review-
Comment 44•7 years ago
|
||
mozreview-review |
Comment on attachment 8967972 [details]
Bug 1432894 - Remove fallback preferences from Marionette.
https://reviewboard.mozilla.org/r/236662/#review243492
::: testing/marionette/components/marionette.js:32
(Diff revision 7)
>
> // Complements -marionette flag for starting the Marionette server.
> // We also set this if Marionette is running in order to start the server
> // again after a Firefox restart.
> const ENV_ENABLED = "MOZ_MARIONETTE";
> +const PREF_ENABLED = "marionette.enabled";
Nothing major but now we have to places where this pref name is defined.
Attachment #8967972 -
Flags: review?(hskupin) → review+
Assignee | ||
Comment 45•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8967971 [details]
Bug 1432894 - Provide shorthands for Marionette prefs.
https://reviewboard.mozilla.org/r/236660/#review243482
> Maybe we should only test if those exist, but not their value. Otherwise if defaults change (like in a different application) the test would also have to be updated.
On the flipside, if someone accidentally changes a default the test
will fail.
> Shouldn't it be enough to test a single pref? Even maybe a less critical one as the enabled state or port? What about the log level?
I don’t think it is good to leave a public-facing API untested.
We don’t have a setter for the log level, as this is not required
in Marionette. The log level can only be set on startup.
Assignee | ||
Comment 46•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8967971 [details]
Bug 1432894 - Provide shorthands for Marionette prefs.
https://reviewboard.mozilla.org/r/236660/#review242638
> This causes a test failure on Android, where "marionette.enabled" is not recognized. Maybe it's also causing all the other retry failures, which cause no single job on Android to pass.
Using default fallbacks should have resolved this.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 51•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8967971 [details]
Bug 1432894 - Provide shorthands for Marionette prefs.
https://reviewboard.mozilla.org/r/236660/#review243482
> On the flipside, if someone accidentally changes a default the test
> will fail.
But what buys us from that? As you said we want to test the new API but not which default value currently is set.
> I don’t think it is good to leave a public-facing API untested.
>
> We don’t have a setter for the log level, as this is not required
> in Marionette. The log level can only be set on startup.
Lets see how this works then. I'm only a bit scary about changing such important prefs.
Comment 52•7 years ago
|
||
mozreview-review |
Comment on attachment 8967971 [details]
Bug 1432894 - Provide shorthands for Marionette prefs.
https://reviewboard.mozilla.org/r/236660/#review243640
Attachment #8967971 -
Flags: review?(hskupin) → review+
Assignee | ||
Comment 53•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8967971 [details]
Bug 1432894 - Provide shorthands for Marionette prefs.
https://reviewboard.mozilla.org/r/236660/#review243482
> But what buys us from that? As you said we want to test the new API but not which default value currently is set.
In the earlier tests for the Branch class we don’t particularly
care about the values. Instead I’ve constructed an artificial
test.* branch with three different types so that the tests hit the
four different code branches for string, number, boolean, and
complex/invalid.
MarionettePrefs returns a concrete class with expected defaults
(2828, "info", false) and I think it _does_ make sense to test these
values. When someone changes the default there will indeed be
another action to also update the tests, but tests are there for a
reason. If someone does a drive-by commit that changes a default,
maybe a failing test will cause the author to consider twice if
making the change is OK.
Does that make sense?
Comment 54•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8967971 [details]
Bug 1432894 - Provide shorthands for Marionette prefs.
https://reviewboard.mozilla.org/r/236660/#review243482
> In the earlier tests for the Branch class we don’t particularly
> care about the values. Instead I’ve constructed an artificial
> test.* branch with three different types so that the tests hit the
> four different code branches for string, number, boolean, and
> complex/invalid.
>
> MarionettePrefs returns a concrete class with expected defaults
> (2828, "info", false) and I think it _does_ make sense to test these
> values. When someone changes the default there will indeed be
> another action to also update the tests, but tests are there for a
> reason. If someone does a drive-by commit that changes a default,
> maybe a failing test will cause the author to consider twice if
> making the change is OK.
>
> Does that make sense?
Lets land and see how it works then.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 59•7 years ago
|
||
Pushed by atolfsen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f37ff744e424
Register marionette.contentListener pref. r=whimboo
https://hg.mozilla.org/integration/autoland/rev/8e9f1887bc89
Rely on MarionetteMainProcess#enabled instead of pref. r=whimboo
https://hg.mozilla.org/integration/autoland/rev/531f8f2af57d
Provide shorthands for Marionette prefs. r=whimboo
https://hg.mozilla.org/integration/autoland/rev/fb56c3c325d3
Remove fallback preferences from Marionette. r=whimboo
Comment 60•7 years ago
|
||
Backed out 4 changesets (bug 1432894) for linting opt test-doc-generate failures.
Backout: https://hg.mozilla.org/integration/autoland/rev/48a09f9f6fd9beabe7ffc8b9800f9756afac25da
Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=fb56c3c325d3c7a79dc11b2dec1c2fc0924f3d3f&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=success&filter-resultStatus=pending&filter-resultStatus=running&selectedJob=175024855
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=175024855&repo=autoland
Exception occurred:
[task 2018-04-22T16:17:04.728Z] File "/builds/worker/checkouts/gecko/obj-x86_64-pc-linux-gnu/_virtualenv/lib/python2.7/site-packages/sphinx_js/jsdoc.py", line 56, in run_jsdoc
[task 2018-04-22T16:17:04.728Z] raise PathsTaken(conflicts)
[task 2018-04-22T16:17:04.728Z] PathsTaken: Your JS code contains multiple documented objects at each of these paths:
[task 2018-04-22T16:17:04.728Z]
[task 2018-04-22T16:17:04.728Z] ./testing/marionette/prefs.MarionetteBranch#enabled
[task 2018-04-22T16:17:04.728Z] ./testing/marionette/prefs.MarionetteBranch#port
[task 2018-04-22T16:17:04.729Z]
[task 2018-04-22T16:17:04.729Z] We won't know which one you're talking about. Using JSDoc tags like @class might help you differentiate them.
[task 2018-04-22T16:17:04.729Z] The full traceback has been saved in /tmp/sphinx-err-KfDBob.log, if you want to report the issue to the developers.
[task 2018-04-22T16:17:04.729Z] Please also report this if it was a user error, so that a better error message can be provided next time.
[task 2018-04-22T16:17:04.729Z] A bug report can be filed in the tracker at <https://github.com/sphinx-doc/sphinx/issues>. Thanks!
[task 2018-04-22T16:17:04.729Z] ./mach: failed to generate documentation:
[task 2018-04-22T16:17:04.729Z] /builds/worker/checkouts/gecko/tools: sphinx return code 1
[taskcluster 2018-04-22 16:17:05.193Z] === Task Finished ===
[taskcluster 2018-04-22 16:17:05.312Z] Artifact "public/docs.tar.gz" not found at "/builds/worker/checkouts/gecko/docs-out/main.tar.gz"
[taskcluster 2018-04-22 16:17:06.087Z] Unsuccessful task run with exit code: 1 completed in 197.564 seconds
Flags: needinfo?(ato)
Assignee | ||
Comment 61•7 years ago
|
||
Apparently jsdoc does not allow you to document both getters and setters.
Flags: needinfo?(ato)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 66•7 years ago
|
||
Pushed by atolfsen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f9b53fc2863e
Register marionette.contentListener pref. r=whimboo
https://hg.mozilla.org/integration/autoland/rev/2244940ed35e
Rely on MarionetteMainProcess#enabled instead of pref. r=whimboo
https://hg.mozilla.org/integration/autoland/rev/885dfa70693d
Provide shorthands for Marionette prefs. r=whimboo
https://hg.mozilla.org/integration/autoland/rev/805a0ef65175
Remove fallback preferences from Marionette. r=whimboo
Comment 67•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/f9b53fc2863e
https://hg.mozilla.org/mozilla-central/rev/2244940ed35e
https://hg.mozilla.org/mozilla-central/rev/885dfa70693d
https://hg.mozilla.org/mozilla-central/rev/805a0ef65175
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox61:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Updated•2 years ago
|
Product: Testing → Remote Protocol
You need to log in
before you can comment on or make changes to this bug.
Description
•