Closed Bug 1247149 Opened 8 years ago Closed 6 years ago

mozscreenshots: Support restricting configurations in sets

Categories

(Testing :: mozscreenshots, defect)

Product:
Testing
Component:
mozscreenshots
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox58 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla58
Tracking Flags:
Tracking Status
firefox58 --- fixed

People

(Reporter: lina, Assigned: rndmustafa, Mentored)

References

Details

Attachments

(2 files, 5 obsolete files)

Bug 1247149 - mozscreenshots: Support restricting configurations in sets.
59 bytes, text/x-review-board-request
jaws
: review+
Details
load-TestRunnerJSM.patch
2.74 KB, patch
Details | Diff | Splinter Review 
For example, specifying `MOZSCREENSHOTS_SETS=Toolbars[allToolbars]` would only run `allToolbars`, and ignore the other configurations.
https://reviewboard.mozilla.org/r/34289/#review30999

::: browser/tools/mozscreenshots/mozscreenshots/extension/TestRunner.jsm:134
(Diff revision 1)
> +            configurations.push(imported[setName].configurations[config]);

We should probably throw (or warn) if you specify a nonexistent configuration in the env var.
Comment on attachment 8717748 [details]
MozReview Request: Bug 1247149 - mozscreenshots: Support restricting configurations in sets. r?MattN

https://reviewboard.mozilla.org/r/34289/#review31015

I think https://mxr.mozilla.org/mozilla-central/source/browser/tools/mozscreenshots/browser_screenshots.js?rev=44ce6138d974&mark=15#13 will need fixing too as I was planning to be able to choose more than one in a jsm like how you can choose more tha one platform for a suite on trychooser e.g. `MOZSCREENSHOTS_SETS=LightweightThemes[noLWT,darkLWT],Tabs`.

If that's significantly harder we can take this for now and supporting multiple later.
Attachment #8717748 - Flags: review?(MattN+bmo)
Also I wonder if we should skip the configuration (e.g. by rejecting applyConfig) when a restriction is applied or don't include it in the combination count at all. I would slightly lean toward skipping as it keeps the image numbers consistent so you can compare a `LightweightThemes[noLWT,darkLWT],Tabs` run with a `LightweightThemes,Tabs` since the numbers in the filenames will be consistent. Though now that I think of it, the comparison script can probably just ignore the image number as the rest of the filename is unique enough.
Component: General → mozscreenshots
Product: Firefox → Testing
Hey kitcambridge,

jaws and I are mentoring some MSU students who are working on mozscreenshots stuff, and I was wondering if one of those students could take this work off of your hands as a Good First Bug. Is that okay?

Cc'ing the student, Rand Mustafa, so he's in the loop.
Flags: needinfo?(kit)
(In reply to Mike Conley (:mconley) (:⚙️) from comment #5)
> Hey kitcambridge,
> 
> jaws and I are mentoring some MSU students who are working on mozscreenshots
> stuff, and I was wondering if one of those students could take this work off
> of your hands as a Good First Bug. Is that okay?

Absolutely! I'm so sorry I lost track of this.
Assignee: kit → nobody
Status: ASSIGNED → NEW
Flags: needinfo?(kit)
Thanks kit!
Assignee: nobody → rndmustafa
Mentor: jaws, mconley
Status: NEW → ASSIGNED
Comment on attachment 8911281 [details]
now checks to see if nonexistent configurations have been specified

https://reviewboard.mozilla.org/r/182772/#review188198

This part looks good, please make the following changes and resubmit for review.

::: browser/tools/mozscreenshots/mozscreenshots/extension/TestRunner.jsm:134
(Diff revision 1)
>          }
>          // Trim the restrictions from the set name.
>          setName = setName.slice(0, match.index);
>          restrictions = match[1].split(",").reduce((set, name) =>
>            set.add(name.trim()), new Set());
> +		  

You should run our lint tool on your code. It will complain about the trailing spaces that got insered here.

To run the tool, you will need to first run `mach eslint --setup`, then you can run `mach eslint path/to/file`.

You can learn more about it at https://developer.mozilla.org/en-US/docs/ESLint

::: browser/tools/mozscreenshots/mozscreenshots/extension/TestRunner.jsm:146
(Diff revision 1)
> +        if(restrictions != null) {
> +          for(let restriction of restrictions){
> +            if( !configurationNames.includes(restriction) ){

You can use Array.filter to do this more concisely.

> if (restrictions &&
>     restrictions.filter(r => !configurationNames.includes(r)).length) {
>   throw new Error(r + " is a nonexistent configuration");
> }
Attachment #8911281 - Flags: review?(jaws) → review-
Comment on attachment 8911282 [details]
Can now choose multiple configurations within a set eg Toolbars[foo,bar].

https://reviewboard.mozilla.org/r/182774/#review188202

Again, this looks good but some minor feedback for you.

::: browser/tools/mozscreenshots/browser_screenshots.js:13
(Diff revision 1)
> + * Finds the index of the first comma that is not enclosed within square brackets.
> + * @param {String} env - the string that needs to be searched
> + * @return {Integer} index of valid comma or -1 if not found.
> + */
> +function findComma(env){
> +  let ignore = false;

Change this to 0 and increment for each '[' and decrement for each ']'. Then only return `i` when a comma is encountered and the nesting depth is 0.

::: browser/tools/mozscreenshots/browser_screenshots.js:39
(Diff revision 1)
> +  let result = [];
> +  
> +  let commaIndex = findComma(env);
> +  while( commaIndex != -1 ){
> +    result.push( env.slice(0, commaIndex).trim() );
> +    env = env.slice(commaIndex+1);

Please run eslint on this too, as I would expect an error for lack of spaces around binary operator.

::: browser/tools/mozscreenshots/browser_screenshots.js:42
(Diff revision 1)
> +  while( commaIndex != -1 ){
> +    result.push( env.slice(0, commaIndex).trim() );
> +    env = env.slice(commaIndex+1);
> +    commaIndex = findComma(env);
> +  }
> +  result.push( env.trim() );

We should call trim() in both code paths. Can you move the trim() call to line 54 inside of the function call?
Attachment #8911282 - Flags: review?(jaws) → review-
Comment on attachment 8912068 [details]
Fixed lint warnings and handled some edge cases.

https://reviewboard.mozilla.org/r/183450/#review189012

Looks good.
Attachment #8912068 - Flags: review?(jaws) → review+
Comment on attachment 8911282 [details]
Can now choose multiple configurations within a set eg Toolbars[foo,bar].

https://reviewboard.mozilla.org/r/182774/#review189014

Changing to r+ now that the issues have been fixed in a latter commit.
Attachment #8911282 - Flags: review- → review+
Comment on attachment 8911281 [details]
now checks to see if nonexistent configurations have been specified

https://reviewboard.mozilla.org/r/182772/#review189016
Attachment #8911281 - Flags: review- → review+
Comment on attachment 8912068 [details]
Fixed lint warnings and handled some edge cases.

https://reviewboard.mozilla.org/r/183450/#review189018

::: npm-shrinkwrap.json:1007
(Diff revision 1)
>      "sprintf-js": {
>        "version": "1.0.3",
>        "resolved": "https://registry.npmjs.org/sprintf-js/-/sprintf-js-1.0.3.tgz",
>        "integrity": "sha1-BOaSb2YolTVPPdAVIDYzuFcpfiw="
>      },
> +    "string_decoder": {

We should revert this change. I'm not sure how it got included in your commit.
Comment on attachment 8913013 [details]
Reverted changes to npm-shrinkwrap.json

https://reviewboard.mozilla.org/r/184370/#review189524
Attachment #8913013 - Flags: review?(jaws) → review+
Comment on attachment 8911280 [details]
Bug 1247149 - mozscreenshots: Support restricting configurations in sets.

https://reviewboard.mozilla.org/r/182768/#review189914

Can you please roll all the commits into one? We generally push to mozreview how we want things to land and we don't like to land intermediate revisions in order to keep a clean history with logically separated commits.

::: browser/tools/mozscreenshots/browser_screenshots.js:15
(Diff revision 3)
> +    if (env[i] === "[")
> +      nestingDepth += 1;
> +    else if (env[i] === "]")
> +      nestingDepth -= 1;
> +    else if (env[i] === "," && nestingDepth === 0) {

Please add curly braces for each of the `if` conditions

::: browser/tools/mozscreenshots/browser_screenshots.js:37
(Diff revision 3)
> + */
> +function splitEnv(env) {
> +  let result = [];
> +
> +  let commaIndex = findComma(env);
> +  while ( commaIndex != -1 ) {

Nit: remove the spaces immediately inside the braces

::: browser/tools/mozscreenshots/browser_screenshots.js:38
(Diff revision 3)
> +function splitEnv(env) {
> +  let result = [];
> +
> +  let commaIndex = findComma(env);
> +  while ( commaIndex != -1 ) {
> +    result.push( env.slice(0, commaIndex).trim() );

Nit: remove the spaces immediately inside the braces

::: browser/tools/mozscreenshots/browser_screenshots.js:42
(Diff revision 3)
> +  while ( commaIndex != -1 ) {
> +    result.push( env.slice(0, commaIndex).trim() );
> +    env = env.slice(commaIndex + 1);
> +    commaIndex = findComma(env);
> +  }
> +  result.push( env.trim() );

Nit: remove the spaces immediately inside the braces

::: browser/tools/mozscreenshots/mozscreenshots/extension/TestRunner.jsm:132
(Diff revision 3)
> +        restrictions = match[1].split(",").reduce((set, name) =>
> +          set.add(name.trim()), new Set());

Split this line at the comma before `new` instead and line the `new` up with the "(set, ") to align the `reduce` arguments vertically

::: browser/tools/mozscreenshots/mozscreenshots/extension/TestRunner.jsm:146
(Diff revision 3)
>          if (!configurationNames.length) {
>            throw new Error(setName + " has no configurations for this environment");
>          }
> +        // Checks to see if nonexistent configuration have been specified
> +        if (restrictions) {
> +          let incorrectConfigs = [...restrictions].filter(r => !configurationNames.includes(r) );

Nit: please remove the space after `(r)`

::: browser/tools/mozscreenshots/mozscreenshots/extension/TestRunner.jsm:151
(Diff revision 3)
> +        let configurations = [];
>          for (let config of configurationNames) {
>            // Automatically set the name property of the configuration object to
>            // its name from the configuration object.
>            imported[setName].configurations[config].name = config;
> +          // Filter restricted configurations.
> +          if (!restrictions || restrictions.has(config)) {
> +            configurations.push(imported[setName].configurations[config]);

Unless I'm misreading the code, the return value should be an array of objects, not an array of arrays so I think `configurations` should be an object, not an array.

It would be great to have an xpcshell unit-test for this method.
Comment on attachment 8911280 [details]
Bug 1247149 - mozscreenshots: Support restricting configurations in sets.

https://reviewboard.mozilla.org/r/182770/#review189948
Attachment #8911280 - Flags: review?(MattN+bmo)
Attachment #8911281 - Attachment is obsolete: true
Attachment #8911282 - Attachment is obsolete: true
Attachment #8912068 - Attachment is obsolete: true
Attachment #8913013 - Attachment is obsolete: true
Comment on attachment 8911280 [details]
Bug 1247149 - mozscreenshots: Support restricting configurations in sets.

https://reviewboard.mozilla.org/r/182770/#review190296

Looks good. Thanks for making those changes, just one more request. Can you write a unit test for this? 

The test can live in /browser/tools/mozscreenshosts/tests/xpcshell/ (you can create the new directories).

You can name the test file `test_testConfigurations.js`. xpcschell tests use the naming convention of `test_*` whereas mochitest tests use the naming convention of `browser_*`.

Then create a `xpcshell.ini` file in the same directory. This will just have `[test_testConfigurations.js]` in it since that is the only test that exists right now and there is no support files or header file to declare.

You will also need to add a .eslintrc.js file to tell ESLint to load our xpcshell-test plugin. This file should be a copy of /toolkit/modules/tests/xpcshell/.eslintrc.js
Attachment #8911280 - Flags: review?(jaws) → review-
Rand, any update?
Flags: needinfo?(rndmustafa)
Comment on attachment 8911280 [details]
Bug 1247149 - mozscreenshots: Support restricting configurations in sets.

https://reviewboard.mozilla.org/r/182770/#review194172

I was able to load TestRunner.jsm through your xpcshell test by making some changes to a few files. I will upload the changes to this bug and you can make the same changes to your patch.

First I needed to mark TestRunner.jsm as a "support-file" within the xpcshell.ini file. This will include the file in the build. TestRunner.jsm wasn't included before because it is part of the extension. Trying to load the extension during the test (like the mochitest tests do) isn't as straightforward since most of the browser architecture isn't available in xpcshell tests.

Then I needed to make a change to TestRunner.jsm to lazily load Screenshot.jsm. TestRunner.jsm was trying to always import Screenshot.jsm, but the path that JSM isn't loaded during the test run and the path that it uses won't work even if we did add it to support-files.

In test_testConfiguration.js I moved "use strict" to the top of the file since it should be declared first, and then I used the "resource://test/" path that is discussed at https://developer.mozilla.org/en-US/docs/Mozilla/QA/Writing_xpcshell-based_unit_tests#Adding_to_your_test to load TestRunner.jsm now that it is local to the test directory.
Attachment #8911280 - Flags: review?(jaws) → review-
Attachment #8717748 - Attachment is obsolete: true
Comment on attachment 8911280 [details]
Bug 1247149 - mozscreenshots: Support restricting configurations in sets.

https://reviewboard.mozilla.org/r/182770/#review196834

::: browser/tools/mozscreenshots/mozscreenshots/extension/TestRunner.jsm:121
(Diff revision 6)
>    /**
> +   * Helper function for loadSets. This filters out the restricted configs from setName.
> +   * This was made a helper function to facilitate xpcshell unit testing.
> +   * @param {String} setName - set name to be filtered e.g. "Toolbars[onlyNavBar,allToolbars]"
> +   * @return {Object} Returns an object with two values: the filtered set name and a set of
> +   * restricted configs.

nit, we should line up the "restricted configs" to the same column as the start of "Returns" above it.

::: browser/tools/mozscreenshots/mozscreenshots/extension/TestRunner.jsm:133
(Diff revision 6)
> +    // Trim the restrictions from the set name.
> +    setName = setName.slice(0, match.index);
> +    let restrictions = match[1].split(",").reduce((set, name) => set.add(name.trim())
> +                                                 , new Set());
> +
> +    return { trimmedSetName: setName, restSet: restrictions };

We should just use `restrictions` as the name of the value. So this would be:

return { trimmedSetName: setName, restrictions };

(In newer JavaScript if the variable name is supplied in the Object constructor with no value following it, the Object will be set to have a member with the same name as the variable.)

::: browser/tools/mozscreenshots/mozscreenshots/extension/TestRunner.jsm:148
(Diff revision 6)
>      for (let setName of setNames) {
> +      let restrictions = null;
> +      if (setName.includes("[")) {
> +        let filteredData = this.filterRestrictions(setName);
> +        setName = filteredData.trimmedSetName;
> +        restrictions = filteredData.restSet;

restrictions = filteredData.restrictions;

::: browser/tools/mozscreenshots/mozscreenshots/extension/TestRunner.jsm:162
(Diff revision 6)
>            throw new Error(setName + " has no configurations for this environment");
>          }
> +        // Checks to see if nonexistent configuration have been specified
> +        if (restrictions) {
> +          let incorrectConfigs = [...restrictions].filter(r => !configurationNames.includes(r));
> +          if (incorrectConfigs.length > 0) {

if (incorrectConfigs.length)

(no need to compare greater than zero, if the value is non-zero it will be truthy)
Attachment #8911280 - Flags: review?(jaws) → review+
Pushed by jwein@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b46909f2b668
mozscreenshots: Support restricting configurations in sets. r=jaws
https://hg.mozilla.org/mozilla-central/rev/b46909f2b668
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox58: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Flags: needinfo?(rndmustafa)
You need to log in before you can comment on or make changes to this bug.