Closed
Bug 1247149
Opened 9 years ago
Closed 8 years ago
mozscreenshots: Support restricting configurations in sets
Categories
(Testing :: mozscreenshots, defect)
Testing
mozscreenshots
Tracking
(firefox58 fixed)
RESOLVED
FIXED
mozilla58
Tracking | Status | |
---|---|---|
firefox58 | --- | fixed |
People
(Reporter: lina, Assigned: rndmustafa, Mentored)
References
Details
Attachments
(2 files, 5 obsolete files)
59 bytes,
text/x-review-board-request
|
jaws
:
review+
|
Details |
2.74 KB,
patch
|
Details | Diff | Splinter Review |
For example, specifying `MOZSCREENSHOTS_SETS=Toolbars[allToolbars]` would only run `allToolbars`, and ignore the other configurations.
Reporter | ||
Comment 1•9 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/34289/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/34289/
Attachment #8717748 -
Flags: review?(MattN+bmo)
Reporter | ||
Comment 2•9 years ago
|
||
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 3•9 years ago
|
||
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)
Comment 4•9 years ago
|
||
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.
Updated•8 years ago
|
Component: General → mozscreenshots
Product: Firefox → Testing
Comment 5•8 years ago
|
||
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)
Reporter | ||
Comment 6•8 years ago
|
||
(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)
Updated•8 years ago
|
Status: NEW → ASSIGNED
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 11•8 years ago
|
||
mozreview-review |
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 12•8 years ago
|
||
mozreview-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 hidden (mozreview-request) |
Comment 14•8 years ago
|
||
mozreview-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 15•8 years ago
|
||
mozreview-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 16•8 years ago
|
||
mozreview-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 17•8 years ago
|
||
mozreview-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 hidden (mozreview-request) |
Comment 19•8 years ago
|
||
mozreview-review |
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 20•8 years ago
|
||
mozreview-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 21•8 years ago
|
||
mozreview-review |
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)
Comment hidden (mozreview-request) |
Assignee | ||
Updated•8 years ago
|
Attachment #8911281 -
Attachment is obsolete: true
Assignee | ||
Updated•8 years ago
|
Attachment #8911282 -
Attachment is obsolete: true
Assignee | ||
Updated•8 years ago
|
Attachment #8912068 -
Attachment is obsolete: true
Assignee | ||
Updated•8 years ago
|
Attachment #8913013 -
Attachment is obsolete: true
Comment 23•8 years ago
|
||
mozreview-review |
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-
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 27•8 years ago
|
||
mozreview-review |
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-
Comment 28•8 years ago
|
||
Updated•8 years ago
|
Attachment #8717748 -
Attachment is obsolete: true
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 31•8 years ago
|
||
mozreview-review |
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+
Comment hidden (mozreview-request) |
Comment 33•8 years ago
|
||
Pushed by jwein@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b46909f2b668
mozscreenshots: Support restricting configurations in sets. r=jaws
![]() |
||
Comment 34•8 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox58:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Updated•8 years ago
|
Flags: needinfo?(rndmustafa)
You need to log in
before you can comment on or make changes to this bug.
Description
•