1088007 - Enhance utils.getProperty to accept multiple properties files

Status: RESOLVED FIXED

(Mozilla QA Graveyard :: Mozmill Tests, defect, P2)

Description

[Daniel Gherasim]

As we discussed in bug 1084333 we should update utils.getProperty() method to be similar as utils.getEntity and accept an array of URLs.

Comment 1

[Daniel Gherasim]

Attachment: v1.patch

This is the fix. In the future we need to only accept array as parameter and update all the tests. Filed bug 1088041 for that.

Comment 2

[Daniel Gherasim]

Attachment: v1.1.patch

Small updates to the test function.

Comment 3

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 8510319 [details] [diff] [review]
v1.1.patch

Review of attachment 8510319 [details] [diff] [review]:
-----------------------------------------------------------------

Small change.

::: lib/tests/testUtils.js
@@ +19,5 @@
> +function testUtils() {
> +  TEST_DATA.properties.forEach(aProp => {
> +    var prop = utils.getProperty(TEST_DATA.urls, aProp);
> +
> +    expect.ok((typeof prop === "string" && prop.length > 0), "Property was recieved");

nit: received

Comment 4

[Daniel Gherasim]

Attachment: v1.2.patch

Comment 5

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

Comment on attachment 8510851 [details] [diff] [review]
v1.2.patch

Review of attachment 8510851 [details] [diff] [review]:
-----------------------------------------------------------------

::: lib/tests/testUtils.js
@@ +9,5 @@
> +
> +const TEST_DATA = {
> +  urls: ["chrome://browser/locale/pageInfo.properties",
> +         "chrome://browser/locale/browser.properties",
> +         "chrome://global/locale/search/search.properties"],

Wrong indentation. Please see the coding styles.

@@ +20,5 @@
> +  TEST_DATA.properties.forEach(aProp => {
> +    var prop = utils.getProperty(TEST_DATA.urls, aProp);
> +
> +    expect.ok((typeof prop === "string" && prop.length > 0),
> +              "Property was received");

Not sure why we have to test this. I think its enough to check if it returns or asserts. Also we don't test an invalid property.

::: lib/utils.js
@@ +335,5 @@
> + *
> + * @returns {string} The value of the requested property
> + */
> +function getProperty(aUrls, aPrefName) {
> +  aUrls = (typeof aUrls === "string") ? [aUrls] : aUrls;

Do not re-assign a new value to a parameter.

Also personally I would have simply updated all the tests, which make use of the method already. That way we would not have to carry around the old behavior.

@@ +337,5 @@
> + */
> +function getProperty(aUrls, aPrefName) {
> +  aUrls = (typeof aUrls === "string") ? [aUrls] : aUrls;
> +
> +  var property = null;

nit: no need for the above blank line.

@@ +348,5 @@
> +    }
> +    catch (ex) { }
> +  });
> +
> +  if (found) {

Why not checking for `if (property == null)` and raising the exception? No need for the else case then. Also I don't see the need for `found`.

Comment 6

[Daniel Gherasim]

Attachment: v2.patch

Update patch based on review notes.
-----------------------------------------------------------------

(In reply to Henrik Skupin (:whimboo) from comment #5)
> Comment on attachment 8510851 [details] [diff] [review]
> v1.2.patch
> 
> Review of attachment 8510851 [details] [diff] [review]:
> -----------------------------------------------------------------
> ::: lib/utils.js
> @@ +335,5 @@
> > + *
> > + * @returns {string} The value of the requested property
> > + */
> > +function getProperty(aUrls, aPrefName) {
> > +  aUrls = (typeof aUrls === "string") ? [aUrls] : aUrls;
> 
> Do not re-assign a new value to a parameter.
> 
> Also personally I would have simply updated all the tests, which make use of
> the method already. That way we would not have to carry around the old
> behavior.
> 

I already filed bug 1084333 for this so we are unblocked and we can do this sperately.

Comment 7

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 8510973 [details] [diff] [review]
v2.patch

Review of attachment 8510973 [details] [diff] [review]:
-----------------------------------------------------------------

All changes requested were addressed.

Comment 8

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

Comment on attachment 8510973 [details] [diff] [review]
v2.patch

Review of attachment 8510973 [details] [diff] [review]:
-----------------------------------------------------------------

Please fix the two remaining items and ask for review from Andreea or Andrei again. With that done r=me.

::: lib/tests/testUtils.js
@@ +23,5 @@
> +
> +function testUtils() {
> +  TEST_DATA.properties.forEach(aProp => {
> +    var prop = utils.getProperty(TEST_DATA.urls, aProp);
> +    expect.ok(prop, "Property was received");

nit: Property has been found.

@@ +35,5 @@
> +  finally {
> +    if (!exceptionRaised) {
> +      assert.fail("Invalid property has not raised an exception");
> +    }
> +  }

This code has to use expect.throws().

::: lib/utils.js
@@ +333,1 @@
>   * @param {string} aPrefName

Please rename it to aPropertyName

Comment 9

[Daniel Gherasim]

Attachment: v3.patch

Thanks!

Comment 10

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 8512572 [details] [diff] [review]
v3.patch

Review of attachment 8512572 [details] [diff] [review]:
-----------------------------------------------------------------

http://hg.mozilla.org/qa/mozmill-tests/rev/f8193ec21381 (default)
Please check backporting.

Comment 11

[Daniel Gherasim]

Attachment: release.patch

Comment 12

[Daniel Gherasim]

Comment on attachment 8512572 [details] [diff] [review]
v3.patch

Applies cleanly on aurora and beta.

Comment 13

[Daniel Gherasim]

Attachment: esr31.patch

Comment 14

[Andrei Eftimie]

Transplanted to Aurora:
https://hg.mozilla.org/qa/mozmill-tests/rev/32ad22f933a2 (mozilla-aurora)

I'd leave this for at least a day on Aurora to make sure nothing regressed. Most libs/tests make use of this particular method.

Comment 15

[Andrei Eftimie]

Comment on attachment 8512572 [details] [diff] [review]
v3.patch

Aurora has been really stable the last days.
Transplanted:
http://hg.mozilla.org/qa/mozmill-tests/rev/152cab4d6827 (mozilla-beta)

Comment 16

[Andrei Eftimie]

Comment on attachment 8512682 [details] [diff] [review]
release.patch

Review of attachment 8512682 [details] [diff] [review]:
-----------------------------------------------------------------

Everything looks fine on release as well:
https://hg.mozilla.org/qa/mozmill-tests/rev/0b714227fcc6 (mozilla-release)

Comment 17

[Andrei Eftimie]

Comment on attachment 8512690 [details] [diff] [review]
esr31.patch

Review of attachment 8512690 [details] [diff] [review]:
-----------------------------------------------------------------

And ESR31 is also green for me.
https://hg.mozilla.org/qa/mozmill-tests/rev/adee13108b46 (mozilla-esr31)