Proposal: add a commit hook requiring magic words in the commit message if you add tests to browser/base/content/test/general/

Gijs Kruitbosch gijskruitbosch at gmail.com
Mon Mar 20 20:52:28 UTC 2017


In a freak coincidence, given the suspected objections to a commit hook, 
I filed https://bugzilla.mozilla.org/show_bug.cgi?id=1348918 earlier 
today to add DOM-style warnings to the browser.ini file (patch is up 
already), as I happened to notice today that people are still adding 
more tests.

Meanwhile, many thanks to Florian, Jared and Mark who have all moved 
various bits and pieces to other directories already.

Gijs

On 20/03/2017 20:35, Gregory Szorc wrote:
> The cost of a hook depends on what level it inspects. Reading 
> changeset metadata is the cheapest and nearly instantaneous. Next is 
> examining the set of file names that changed. The slowest are hooks 
> that need to examine content.
>
> With the movement towards Autoland, latency of pushes becomes a lesser 
> concern and the biggest concern for hooks is ongoing maintenance 
> costs. A parallel concern is ensuring failures are caught fast - as 
> early in the development process as possible. We don't want developers 
> only finding out about bustage at landing time.
>
> On Wed, Feb 22, 2017 at 6:12 AM, Ryan VanderMeulen 
> <rvandermeulen at mozilla.com <mailto:rvandermeulen at mozilla.com>> wrote:
>
>     I suspect gps will have opinions about this when he returns from
>     PTO. Basically, I think the main concern is that commit hooks have
>     a performance penalty for every push and should therefore be
>     avoided where possible.
>
>     -Ryan
>
>     On Wed, Feb 22, 2017 at 9:09 AM, Jared Wein <jwein at mozilla.com
>     <mailto:jwein at mozilla.com>> wrote:
>
>         On a recent Win7 Opt bc-7 run, the following tests took over
>         1s each in /browser/base/content/test/general. The number at
>         the beginning is the amount of milliseconds for the respective
>         test.
>
>         msunu at DESKTOP-I07UH4B /c/fx
>         $ sort dump.txt -n -r
>         32413 took |
>         browser/base/content/test/general/browser_audioTabIcon.js
>         18188 took |
>         browser/base/content/test/general/browser_bookmark_popup.js
>         15910 took |
>         browser/base/content/test/general/browser_parsable_script.js
>         13190 took |
>         browser/base/content/test/general/browser_bug1299667.js
>         12157 took |
>         browser/base/content/test/general/browser_sanitizeDialog.js
>         11302 took |
>         browser/base/content/test/general/browser_bug553455.js
>         10983 took |
>         browser/base/content/test/general/browser_selectpopup.js
>         10472 took | browser/base/content/test/general/browser_syncui.js
>         10422 took |
>         browser/base/content/test/general/browser_newWindowDrop.js
>         9102 took |
>         browser/base/content/test/general/browser_extension_permissions.js
>         8281 took |
>         browser/base/content/test/general/browser_extension_update_background.js
>         7629 took | browser/base/content/test/general/browser_aboutHome.js
>         7479 took |
>         browser/base/content/test/general/browser_extension_update_interactive.js
>         6518 took |
>         browser/base/content/test/general/browser_contentSearchUI.js
>         6015 took |
>         browser/base/content/test/general/browser_aboutAccounts.js
>         5397 took |
>         browser/base/content/test/general/browser_contentAltClick.js
>         5077 took |
>         browser/base/content/test/general/browser_tab_drag_drop_perwindow.js
>         4520 took |
>         browser/base/content/test/general/browser_sanitize-timespans.js
>         4226 took |
>         browser/base/content/test/general/browser_trackingUI_telemetry.js
>         4209 took |
>         browser/base/content/test/general/browser_windowactivation.js
>         4094 took |
>         browser/base/content/test/general/browser_web_channel.js
>         3408 took | browser/base/content/test/general/browser_bug676619.js
>         3148 took |
>         browser/base/content/test/general/browser_fullscreen-window-open.js
>         3059 took |
>         browser/base/content/test/general/browser_printpreview.js
>         2987 took | browser/base/content/test/general/browser_bug422590.js
>         2924 took |
>         browser/base/content/test/general/browser_trackingUI_4.js
>         2893 took | browser/base/content/test/general/browser_bug575561.js
>         2878 took |
>         browser/base/content/test/general/browser_relatedTabs.js
>         2862 took | browser/base/content/test/general/browser_ctrlTab.js
>         2859 took | browser/base/content/test/general/browser_bug590206.js
>         2763 took |
>         browser/base/content/test/general/browser_refreshBlocker.js
>         2718 took |
>         browser/base/content/test/general/browser_trackingUI_5.js
>         2702 took |
>         browser/base/content/test/general/browser_trackingUI_1.js
>         2659 took |
>         browser/base/content/test/general/browser_aboutCertError.js
>         2630 took |
>         browser/base/content/test/general/browser_extension_sideloading.js
>         2550 took |
>         browser/base/content/test/general/browser_storagePressure_notification.js
>         2441 took |
>         browser/base/content/test/general/browser_ssl_error_reports.js
>         2365 took |
>         browser/base/content/test/general/browser_temporary_permissions_navigation.js
>         2295 took |
>         browser/base/content/test/general/browser_bug767836_perwindowpb.js
>         2279 took | browser/base/content/test/general/browser_bug495058.js
>         2185 took |
>         browser/base/content/test/general/browser_bug763468_perwindowpb.js
>         2147 took |
>         browser/base/content/test/general/browser_beforeunload_duplicate_dialogs.js
>         2138 took |
>         browser/base/content/test/general/browser_newwindow_focus.js
>         2067 took |
>         browser/base/content/test/general/browser_overflowScroll.js
>         2016 took |
>         browser/base/content/test/general/browser_getshortcutoruri.js
>         2014 took |
>         browser/base/content/test/general/browser_save_link_when_window_navigates.js
>         1977 took | browser/base/content/test/general/browser_bug592338.js
>         1976 took |
>         browser/base/content/test/general/browser_save_link-perwindowpb.js
>         1895 took |
>         browser/base/content/test/general/browser_temporary_permissions_tabs.js
>         1894 took |
>         browser/base/content/test/general/browser_temporary_permissions.js
>         1882 took |
>         browser/base/content/test/general/browser_documentnavigation.js
>         1855 took |
>         browser/base/content/test/general/browser_permissions.js
>         1818 took |
>         browser/base/content/test/general/browser_tab_dragdrop.js
>         1807 took |
>         browser/base/content/test/general/browser_trackingUI_2.js
>         1788 took |
>         browser/base/content/test/general/browser_parsable_css.js
>         1776 took | browser/base/content/test/general/browser_bug719271.js
>         1752 took |
>         browser/base/content/test/general/browser_datachoices_notification.js
>         1670 took |
>         browser/base/content/test/general/browser_newTabDrop.js
>         1670 took |
>         browser/base/content/test/general/browser_misused_characters_in_strings.js
>         1638 took |
>         browser/base/content/test/general/browser_tab_dragdrop2.js
>         1608 took |
>         browser/base/content/test/general/browser_e10s_switchbrowser.js
>         1567 took |
>         browser/base/content/test/general/browser_e10s_chrome_process.js
>         1444 took |
>         browser/base/content/test/general/browser_contextmenu_input.js
>         1428 took | browser/base/content/test/general/browser_bug462673.js
>         1357 took |
>         browser/base/content/test/general/browser_domFullscreen_fullscreenMode.js
>         1353 took |
>         browser/base/content/test/general/browser_tabkeynavigation.js
>         1274 took |
>         browser/base/content/test/general/browser_aboutHome_wrapsCorrectly.js
>         1232 took |
>         browser/base/content/test/general/browser_private_browsing_window.js
>         1232 took |
>         browser/base/content/test/general/browser_favicon_change_not_in_document.js
>         1213 took |
>         browser/base/content/test/general/browser_tab_detach_restore.js
>         1129 took | browser/base/content/test/general/browser_bug735471.js
>         1087 took |
>         browser/base/content/test/general/browser_zbug569342.js
>         1081 took |
>         browser/base/content/test/general/browser_alltabslistener.js
>         1056 took | browser/base/content/test/general/browser_bug623893.js
>         1054 took |
>         browser/base/content/test/general/browser_contextmenu_childprocess.js
>         1042 took | browser/base/content/test/general/browser_tabDrop.js
>         1015 took |
>         browser/base/content/test/general/browser_offlineQuotaNotification.js
>
>         These 79 tests take 5m30s to run, not including time between
>         tests which is notable on b-c runs. In comparison, the full
>         test suite (238 tests in this case) takes 6m23s to run.
>         Treeherder reports the full duration as 12 minutes, so you can
>         see the amount of overhead we've got.
>
>         I'm fine with the commit hook. The list above should give us a
>         good priority view on what tests should be moved first.
>
>         Cheers,
>         Jared
>
>         On Wed, Feb 22, 2017 at 6:34 AM, Michael de Boer
>         <mdeboer at mozilla.com <mailto:mdeboer at mozilla.com>> wrote:
>
>             +1
>
>>             On 22 Feb 2017, at 11:09, Gijs Kruitbosch
>>             <gijskruitbosch at gmail.com
>>             <mailto:gijskruitbosch at gmail.com>> wrote:
>>
>>             Hello firefox-dev,
>>
>>             A while back I tried to start moving (browser) mochitests
>>             out of browser/base/content/test/general/ to more topical
>>             directories. To copy my reasoning from the original metabug:
>>
>>
>>>             It takes ages to run [locally], there's a lot of tests
>>>             that can influence each other, it's hard to isolate a
>>>             problem if tests do influence each other, it's hard to
>>>             run tests relevant for the change you're making, it
>>>             takes long on infra, if something breaks (cough bug
>>>             1253956 cough) everything gets mass-disabled... there
>>>             are a lot of reasons to not want to have a directory
>>>             with over 300 tests in.
>>
>>             To add to that: browser mochitests are some of our
>>             slowest tests. As far as I can tell, the mochitest-bc
>>             chunk on infra that contains general/ right now *runs no
>>             other directories at all*. On linux e10s debug
>>             <https://public-artifacts.taskcluster.net/KIBQPE3_QZCAnjxL8jd9zA/0/public/logs/live_backing.log>,
>>             that directory takes 50 minutes (3400s) to run.
>>
>>             There are a lot more topical subdirs now, and there's
>>             really no good reason, IMO, to keep adding random tests
>>             to this directory. Which does seem to keep happening:
>>             while the number of tests was 266 after I moved 40-odd
>>             tests to the urlbar/ directory 10 months ago, and more
>>             tests were moved out of general/ since, the number of
>>             tests in general/ right now is 267 (we'll run a ~90%
>>             subset of those depending on platform, debug on/off and
>>             e10s on/off).
>>
>>             I'd like to add a commit/push hook that makes adding more
>>             tests impossible unless you include some magic words,
>>             similar to the webidl DOM peer review hooks.
>>
>>             Would anyone object to this happening?
>>
>             Heck no.
>
>             Cheers,
>
>             Mike.
>
>             _______________________________________________
>             firefox-dev mailing list
>             firefox-dev at mozilla.org <mailto:firefox-dev at mozilla.org>
>             https://mail.mozilla.org/listinfo/firefox-dev
>             <https://mail.mozilla.org/listinfo/firefox-dev>
>
>
>
>         _______________________________________________
>         firefox-dev mailing list
>         firefox-dev at mozilla.org <mailto:firefox-dev at mozilla.org>
>         https://mail.mozilla.org/listinfo/firefox-dev
>         <https://mail.mozilla.org/listinfo/firefox-dev>
>
>
>
>     _______________________________________________
>     firefox-dev mailing list
>     firefox-dev at mozilla.org <mailto:firefox-dev at mozilla.org>
>     https://mail.mozilla.org/listinfo/firefox-dev
>     <https://mail.mozilla.org/listinfo/firefox-dev>
>
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.mozilla.org/pipermail/firefox-dev/attachments/20170320/0e11bc5b/attachment.html>


More information about the firefox-dev mailing list