New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 735587 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Please use my @vewd.com account ins...
Closed: Jul 2017
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

base::LaunchUnitTestsSerially should not be overridable via cmdline

Reported by most...@opera.com, Jun 21 2017

Issue description

The documentation for base::LaunchUnitTestsSerially says that it "always runs tests serially", but if the user happens to specify switches::kTestLauncherJobs on the command line then the corresponding unit tests can be run in parallel.  This is counter-intuitive, in my opinion we should honor the contract in the comment.
 

Comment 2 by most...@opera.com, Jun 21 2017

Summary: base::LaunchUnitTestsSerially should not be overridable via cmdline (was: base::LaunchUnitTestsSerially should not be overridable by cmdline)
Project Member

Comment 3 by bugdroid1@chromium.org, Jun 23 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/d5b40133c57d80b9368a832ee8ad497d34aab392

commit d5b40133c57d80b9368a832ee8ad497d34aab392
Author: Mostyn Bramley-Moore <mostynb@opera.com>
Date: Fri Jun 23 19:34:49 2017

base::LaunchUnitTestsSerially should not be overridable

base::LaunchUnitTestsSerially should ignore the --test-launcher-jobs
command line switch, and never use parallel jobs.

BUG= 735587 

Change-Id: I7e455a805b898f12e7adc91a41feb9627e008964
Reviewed-on: https://chromium-review.googlesource.com/543344
Commit-Queue: Mostyn Bramley-Moore <mostynb@opera.com>
Reviewed-by: Paweł Hajdan Jr. <phajdan.jr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#481990}
[modify] https://crrev.com/d5b40133c57d80b9368a832ee8ad497d34aab392/base/test/launcher/test_launcher.cc
[modify] https://crrev.com/d5b40133c57d80b9368a832ee8ad497d34aab392/base/test/launcher/test_launcher.h
[modify] https://crrev.com/d5b40133c57d80b9368a832ee8ad497d34aab392/base/test/launcher/unit_test_launcher.cc
[modify] https://crrev.com/d5b40133c57d80b9368a832ee8ad497d34aab392/base/test/launcher/unit_test_launcher.h

Comment 4 by most...@opera.com, Jun 23 2017

Status: Fixed (was: Started)
Project Member

Comment 5 by bugdroid1@chromium.org, Jun 26 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/ec515d037019e5a1ceace133a5bdfb005cb5caf5

commit ec515d037019e5a1ceace133a5bdfb005cb5caf5
Author: Henrik Kjellander <kjellander@chromium.org>
Date: Mon Jun 26 22:30:52 2017

Revert "base::LaunchUnitTestsSerially should not be overridable"

This reverts commit d5b40133c57d80b9368a832ee8ad497d34aab392.

Reason for revert: Breaks all WebRTC browser_tests that runs on real audio and video devices (tests marked as MANUAL), since they rely on the behavior of --test-launcher-jobs=1. With this CL those tests started running in parallel, which doesn't work since each test needs exclusive access to the webcam. This also made all the perf measurements become very unstable.
See  crbug.com/736837  for examples.

I'll be happy to update our bots to use another flag, but I need to revert this now to bring our perf bots back into a stable state asap. Please reach out to me before relanding this.

We run our tests with --run-manual --ui-test-action-max-timeout=350000 --test-launcher-jobs=1 --test-launcher-bot-mode --test-launcher-print-test-stdio=always
today. I notice there's also --single-process-tests available, but I'm not convinced that means they don't run in parallel, which is why I'm reverting this to be sure for now.

Original change's description:
> base::LaunchUnitTestsSerially should not be overridable
> 
> base::LaunchUnitTestsSerially should ignore the --test-launcher-jobs
> command line switch, and never use parallel jobs.
> 
> BUG= 735587 
> 
> Change-Id: I7e455a805b898f12e7adc91a41feb9627e008964
> Reviewed-on: https://chromium-review.googlesource.com/543344
> Commit-Queue: Mostyn Bramley-Moore <mostynb@opera.com>
> Reviewed-by: Paweł Hajdan Jr. <phajdan.jr@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#481990}

TBR=phajdan.jr@chromium.org,mostynb@opera.com

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug:  735587 ,  736837 
Change-Id: Ief9b133c9daab91dec6bcb7a920f12211d03e622
Reviewed-on: https://chromium-review.googlesource.com/549295
Reviewed-by: Henrik Kjellander <kjellander@chromium.org>
Commit-Queue: Henrik Kjellander <kjellander@chromium.org>
Cr-Commit-Position: refs/heads/master@{#482438}
[modify] https://crrev.com/ec515d037019e5a1ceace133a5bdfb005cb5caf5/base/test/launcher/test_launcher.cc
[modify] https://crrev.com/ec515d037019e5a1ceace133a5bdfb005cb5caf5/base/test/launcher/test_launcher.h
[modify] https://crrev.com/ec515d037019e5a1ceace133a5bdfb005cb5caf5/base/test/launcher/unit_test_launcher.cc
[modify] https://crrev.com/ec515d037019e5a1ceace133a5bdfb005cb5caf5/base/test/launcher/unit_test_launcher.h

Comment 6 by most...@opera.com, Jun 27 2017

Status: Assigned (was: Fixed)
Project Member

Comment 8 by bugdroid1@chromium.org, Jul 1 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/d5588cf4ffb5a932dcbfd87cd7761d390c790ea9

commit d5588cf4ffb5a932dcbfd87cd7761d390c790ea9
Author: Mostyn Bramley-Moore <mostynb@opera.com>
Date: Sat Jul 01 12:57:18 2017

(reland) base::LaunchUnitTestsSerially should not be overridable by cmdline

(reland) base::LaunchUnitTestsSerially should ignore the --test-launcher-jobs
command line switch, and never use parallel jobs.  To achieve this, change
callers of base::TestLauncher to pass the requested number of parallel test
jobs, instead of the default number of parallel test jobs.

And while we're at it, let's remove TestLauncherDelegate's
AdjustDefaultParallelJobs method, which no longer has any users.

BUG= 735587 , 736837 
TBR=rdevlin.cronin@chromium.org

Change-Id: I4085fb21c1dce467527210407e3913ff3b5e3bc6
Reviewed-on: https://chromium-review.googlesource.com/549342
Commit-Queue: Mostyn Bramley-Moore <mostynb@opera.com>
Reviewed-by: Henrik Kjellander <kjellander@chromium.org>
Reviewed-by: Alok Priyadarshi <alokp@chromium.org>
Reviewed-by: Paweł Hajdan Jr. <phajdan.jr@chromium.org>
Reviewed-by: Alex Clarke <alexclarke@chromium.org>
Cr-Commit-Position: refs/heads/master@{#483909}
[modify] https://crrev.com/d5588cf4ffb5a932dcbfd87cd7761d390c790ea9/base/test/launcher/test_launcher.cc
[modify] https://crrev.com/d5588cf4ffb5a932dcbfd87cd7761d390c790ea9/base/test/launcher/test_launcher.h
[modify] https://crrev.com/d5588cf4ffb5a932dcbfd87cd7761d390c790ea9/base/test/launcher/unit_test_launcher.cc
[modify] https://crrev.com/d5588cf4ffb5a932dcbfd87cd7761d390c790ea9/base/test/launcher/unit_test_launcher.h
[modify] https://crrev.com/d5588cf4ffb5a932dcbfd87cd7761d390c790ea9/chrome/test/base/browser_perf_tests_main.cc
[modify] https://crrev.com/d5588cf4ffb5a932dcbfd87cd7761d390c790ea9/chrome/test/base/browser_tests_main.cc
[modify] https://crrev.com/d5588cf4ffb5a932dcbfd87cd7761d390c790ea9/chrome/test/base/browser_tests_main_chromeos.cc
[modify] https://crrev.com/d5588cf4ffb5a932dcbfd87cd7761d390c790ea9/chrome/test/base/chrome_test_launcher.cc
[modify] https://crrev.com/d5588cf4ffb5a932dcbfd87cd7761d390c790ea9/chrome/test/base/chrome_test_launcher.h
[modify] https://crrev.com/d5588cf4ffb5a932dcbfd87cd7761d390c790ea9/chrome/test/base/interactive_ui_tests_main.cc
[modify] https://crrev.com/d5588cf4ffb5a932dcbfd87cd7761d390c790ea9/chrome/test/base/mash_browser_tests_main.cc
[modify] https://crrev.com/d5588cf4ffb5a932dcbfd87cd7761d390c790ea9/chromecast/app/cast_test_launcher.cc
[modify] https://crrev.com/d5588cf4ffb5a932dcbfd87cd7761d390c790ea9/content/public/test/test_launcher.cc
[modify] https://crrev.com/d5588cf4ffb5a932dcbfd87cd7761d390c790ea9/content/public/test/test_launcher.h
[modify] https://crrev.com/d5588cf4ffb5a932dcbfd87cd7761d390c790ea9/content/test/content_test_launcher.cc
[modify] https://crrev.com/d5588cf4ffb5a932dcbfd87cd7761d390c790ea9/extensions/shell/test/shell_tests_main.cc
[modify] https://crrev.com/d5588cf4ffb5a932dcbfd87cd7761d390c790ea9/headless/test/headless_test_launcher.cc

Comment 9 by most...@opera.com, Jul 6 2017

Status: Started (was: Assigned)
Project Member

Comment 10 by bugdroid1@chromium.org, Jul 14 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/f1b412c80b1bbecf4f6bc663a6e7e061b71a6269

commit f1b412c80b1bbecf4f6bc663a6e7e061b71a6269
Author: Mostyn Bramley-Moore <mostynb@opera.com>
Date: Fri Jul 14 16:33:54 2017

--gtest_filter should only change NumParallelJobs' default value

BUG= 741927 , 735587 

Change-Id: Ib95bd85e2f1ad14e27ba8fa76f8b52f8e137e830
Reviewed-on: https://chromium-review.googlesource.com/569159
Reviewed-by: Paweł Hajdan Jr. <phajdan.jr@chromium.org>
Commit-Queue: Mostyn Bramley-Moore <mostynb@opera.com>
Cr-Commit-Position: refs/heads/master@{#486773}
[modify] https://crrev.com/f1b412c80b1bbecf4f6bc663a6e7e061b71a6269/base/test/launcher/test_launcher.cc

Comment 11 by most...@opera.com, Jul 17 2017

Status: Fixed (was: Started)

Sign in to add a comment