Closed Bug 1460912 Opened 2 years ago Closed 2 years ago

Use base profiles from testing/profiles in reftest

Categories

(Testing :: Reftest, enhancement)

Version 3
enhancement
Not set

Tracking

(firefox62 fixed)

RESOLVED FIXED
mozilla62
Tracking Status
firefox62 --- fixed

People

(Reporter: ahal, Assigned: ahal)

References

Details

Attachments

(7 files)

Follow-up work from bug 1451159.

This will move reftest preferences in layout/tools/reftest-preferences.js into the new testing/profiles system.
Assignee: nobody → ahalberstadt
Status: NEW → ASSIGNED
Attachment #8976323 - Flags: review?(rwood)
Attachment #8976338 - Flags: review?(rwood)
Attachment #8976324 - Flags: review?(rwood)
Attachment #8976325 - Flags: review?(rwood)
Attachment #8976326 - Flags: review?(gbrown)
Attachment #8976327 - Flags: review?(gbrown)
Attachment #8976328 - Flags: review?(gbrown)
Comment on attachment 8976323 [details]
Bug 1460912 - [testing/profiles] Add ability to diff and show multiple profiles with '+'

https://reviewboard.mozilla.org/r/244504/#review250696

LGTM and tested it out locally on OSX:

./mach python testing/profiles/profile diff perf+common raptor
==> only diff is the pref for non_blank_paint

./mach python testing/profiles/profile diff raptor+perf+common perf+common
==> only diff is the pref for non_blank_paint

./mach python testing/profiles/profile diff raptor+perf+common common+raptor+perf
==> no diffs

./mach python testing/profiles/profile show common+perf > ../Desktop/1.txt
./mach python testing/profiles/profile show raptor > ../Desktop/2.txt
diff ../Desktop/1.txt ../Desktop/2.txt
60a61
dom.performance.time_to_non_blank_paint.enabled: True

::: testing/profiles/profile:13
(Diff revision 2)
>  
>  # The beginning of this script is both valid shell and valid python,
>  # such that the script starts with the shell and is reexecuted python
>  '''which' mach > /dev/null 2>&1 && exec mach python "$0" "$@" ||
> -echo "mach not found, either add it to your \$PATH or run this script via ./mach python testing/profiles/profile"; exit
> +echo "mach not found, either add it to your \$PATH or run this script via ./mach python testing/profiles/profile"; exit  # noqa
>  '''

just curious what the '# noqa' comment is?
Attachment #8976323 - Flags: review?(rwood) → review+
Comment on attachment 8976338 [details]
Bug 1460912 - [testing/profiles] Add --format options to ./profile diff

https://reviewboard.mozilla.org/r/244516/#review250722

LGTM, one comment below

::: testing/profiles/profile:244
(Diff revision 1)
>                               help="Path to the first profile or suite name to diff.")
>      diff_parser.add_argument('b', metavar='B',
>                               help="Path to the second profile or suite name to diff.")
> +    diff_parser.add_argument('-f', '--format', dest='fmt', default='pretty',
> +                             choices=['pretty', 'json', 'names'],
> +                             help="Format to dump diff in (default: pretty)")

I find getting help in the tool itself is a bit awkward.

./mach python testing/profiles/profile -- --help
usage: profile [-h] {diff,sort,show} ...

positional arguments:
  {diff,sort,show}

optional arguments:
  -h, --help        show this help message and exit

So then I try to get help for 'diff' so I can see the '--format' option, so I try:

./mach python testing/profiles/profile -h diff
usage: mach [global arguments] python [command arguments]

Run Python.
...

but that gives help for the mach python command, not the 'profile diff' command.
Attachment #8976338 - Flags: review?(rwood) → review+
Comment on attachment 8976338 [details]
Bug 1460912 - [testing/profiles] Add --format options to ./profile diff

https://reviewboard.mozilla.org/r/244516/#review250730

::: testing/profiles/profile:60
(Diff revision 1)
> +    'pretty': (
> +        '{pref}: {value}',
> +        '{pref}: {value_a} => {value_b}'
> +    ),
> +}
> +

One other thing I noticed. If I do:

./mach python testing/profiles/profile diff common+perf raptor

The console output is nicely alphabetical within each section (delete, insert, same).

However if I do:

./mach python testing/profiles/profile diff common+perf raptor --format json

Inside the json sections (i.e. delete, insert, same) the prefs seem random and aren't in alphabetical order which is a bit annoying.
Comment on attachment 8976323 [details]
Bug 1460912 - [testing/profiles] Add ability to diff and show multiple profiles with '+'

https://reviewboard.mozilla.org/r/244504/#review250696

> just curious what the '# noqa' comment is?

# noqa tells flake8 to ignore that specific line
Comment on attachment 8976324 [details]
Bug 1460912 - [testing/profiles] Add option to limit ./profile diff to a specified key

https://reviewboard.mozilla.org/r/244506/#review250778

Yep LGTM and works great
Attachment #8976324 - Flags: review?(rwood) → review+
Comment on attachment 8976327 [details]
Bug 1460912 - [testing/profiles] Sort testing/profiles/reftest/user.js

https://reviewboard.mozilla.org/r/244512/#review250790
Attachment #8976327 - Flags: review?(gbrown) → review+
Comment on attachment 8976325 [details]
Bug 1460912 - [testing/profiles] Add a ./profile rm subcommand for removing prefs from a profile

https://reviewboard.mozilla.org/r/244508/#review250792

LGTM, tested locally on OSX:

rwood$ cat ../Desktop/pref_to_remove.txt
dom.performance.time_to_non_blank_paint.enabled

rwood$ cat testing/profiles/raptor/user.js
// Preferences file used by the raptor harness
/* globals user_pref */
user_pref("dom.performance.time_to_non_blank_paint.enabled", true);

rwood$ ./mach python testing/profiles/profile rm raptor --pref-file ../Desktop/pref_to_remove.txt

rwood$ cat testing/profiles/raptor/user.js
// Preferences file used by the raptor harness
/* globals user_pref */

And reverted raptor prefs, then:

rwood$ cat testing/profiles/raptor/user.js
// Preferences file used by the raptor harness
/* globals user_pref */
user_pref("dom.performance.time_to_non_blank_paint.enabled", true);

rwood$ ./mach python testing/profiles/profile diff common+perf raptor -f names -k insert | ./mach python testing/profiles/profile rm raptor

rwood$ cat testing/profiles/raptor/user.js
// Preferences file used by the raptor harness
/* globals user_pref */

Nice!
Attachment #8976325 - Flags: review?(rwood) → review+
Does this change the preference settings used in reftest?  If so, I'd be interested in reviewing a diff of the settings.
Flags: needinfo?(ahal)
Comment on attachment 8976328 [details]
Bug 1460912 - [testing/profiles] Use the 'common' profile in reftest

https://reviewboard.mozilla.org/r/244514/#review250794

It is sad to see how many prefs are not common...but that's another matter, and this is a great step in the right direction.
Attachment #8976328 - Flags: review?(gbrown) → review+
Comment on attachment 8976326 [details]
Bug 1460912 - [reftest] Use base profiles in reftest

https://reviewboard.mozilla.org/r/244510/#review250798
Attachment #8976326 - Flags: review?(gbrown) → review+
(In reply to David Baron :dbaron: ⌚UTC-7 from comment #21)
> Does this change the preference settings used in reftest?  If so, I'd be
> interested in reviewing a diff of the settings.

No, I made sure the set of prefs were identical by dumping the profile immediately before first run with and without my patch. For extra caution I'll do it again before pushing to make sure I didn't mishandle a conflict.

Tangentially related, it might be a good idea if we could upload the user.js files from our test harnesses as artifacts for each run. That way we could run diffs by pointing to try urls.
Flags: needinfo?(ahal)
Comment on attachment 8976338 [details]
Bug 1460912 - [testing/profiles] Add --format options to ./profile diff

https://reviewboard.mozilla.org/r/244516/#review250730

> One other thing I noticed. If I do:
> 
> ./mach python testing/profiles/profile diff common+perf raptor
> 
> The console output is nicely alphabetical within each section (delete, insert, same).
> 
> However if I do:
> 
> ./mach python testing/profiles/profile diff common+perf raptor --format json
> 
> Inside the json sections (i.e. delete, insert, same) the prefs seem random and aren't in alphabetical order which is a bit annoying.

Good idea, I can add sorted keys in.

p.s here's a nifty little trick for the next time you come across hard to read json:

    ./profile diff common+perf raptor --format json | python -m json.tool
    
That will both indent and sort any json output. I have `python -m json.tool` aliased to `indent` in my shell.
Comment on attachment 8976338 [details]
Bug 1460912 - [testing/profiles] Add --format options to ./profile diff

https://reviewboard.mozilla.org/r/244516/#review250722

> I find getting help in the tool itself is a bit awkward.
> 
> ./mach python testing/profiles/profile -- --help
> usage: profile [-h] {diff,sort,show} ...
> 
> positional arguments:
>   {diff,sort,show}
> 
> optional arguments:
>   -h, --help        show this help message and exit
> 
> So then I try to get help for 'diff' so I can see the '--format' option, so I try:
> 
> ./mach python testing/profiles/profile -h diff
> usage: mach [global arguments] python [command arguments]
> 
> Run Python.
> ...
> 
> but that gives help for the mach python command, not the 'profile diff' command.

Yeah, this is a side-effect of running `mach python <script>`. I think the only way to fix it would be to turn the profile script into a full blown mach command.

Maybe that's how I should have implemented it from the start, but if we do go that route I'd prefer it happen in a follow up. So if you don't mind I'd like to drop this issue for this bug.
Pushed by ahalberstadt@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2600a2947d25
[testing/profiles] Add ability to diff and show multiple profiles with '+' r=rwood
https://hg.mozilla.org/integration/autoland/rev/0b6ae2cdbeba
[testing/profiles] Add --format options to ./profile diff r=rwood
https://hg.mozilla.org/integration/autoland/rev/794cca0138c4
[testing/profiles] Add option to limit ./profile diff to a specified key r=rwood
https://hg.mozilla.org/integration/autoland/rev/631008a3bf29
[testing/profiles] Add a ./profile rm subcommand for removing prefs from a profile r=rwood
https://hg.mozilla.org/integration/autoland/rev/24fc34c2bb34
[reftest] Use base profiles in reftest r=gbrown
https://hg.mozilla.org/integration/autoland/rev/d8e9cd275290
[testing/profiles] Sort testing/profiles/reftest/user.js r=gbrown
https://hg.mozilla.org/integration/autoland/rev/451f6e440971
[testing/profiles] Use the 'common' profile in reftest r=gbrown
You need to log in before you can comment on or make changes to this bug.