Skip to content

Balance unit tests across multiple threads #18956

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Oct 11, 2017
Merged

Conversation

weswigham
Copy link
Member

@weswigham weswigham commented Oct 4, 2017

Previously we just reserved one thread for unit tests; but they now take longer on their own than the average execution time of all the other threads - making them the critical path for testing. This change allows us to execute (and profile!) each suite separately; after an initial run to gather profiling data on the unit tests, this managed to bring my total runtime down by a few seconds (~6s-10s), and perhaps more importantly, exposes gems like this (numbers are ms):

  [ 'tsrunner-unittest://extractConstants', 46651 ],
  [ 'tsrunner-unittest://extractFunctions', 44132 ],
  [ 'tsrunner-unittest://tsc-watch program updates', 16254 ],

and in a second run, to show that these numbers seem somewhat consistent:

  [ 'tsrunner-unittest://extractConstants', 44972 ],
  [ 'tsrunner-unittest://extractFunctions', 43215 ],
  [ 'tsrunner-unittest://tsc-watch program updates', 14786 ],

Our three longest-running test suites are unittest suites, and are recently modified/created. I was wondering why test time went up so much recently, and now I know why. 🐱 If I felt comfortable skipping them, I could save almost 2 minutes of total wall-clock time (dived across 8 threads - so likely ~13s of real time) just by skipping those tests.

@weswigham weswigham requested review from sandersn and mhegazy October 4, 2017 23:03
@@ -1,14 +1,14 @@
/// <reference path="./host.ts" />
/// <reference path="./worker.ts" />
namespace Harness.Parallel {
export type ParallelTestMessage = { type: "test", payload: { runner: TestRunnerKind, file: string } } | never;
export type ParallelTestMessage = { type: "test", payload: { runner: TestRunnerKind | "unittest", file: string } } | never;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not add "unittest" to TestRunnerKind?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We use TestRunnerKind outside the parallel runner to mean "anything for which we have a mapping to a runner class". Unfortunately, the logic doesn't generalize well to normal unit tests (as they don't conform to that API).

@weswigham weswigham merged commit d0168af into master Oct 11, 2017
@weswigham weswigham deleted the loadbalance-unit-tests branch October 11, 2017 00:59
@microsoft microsoft locked and limited conversation to collaborators Jun 14, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants