-
Notifications
You must be signed in to change notification settings - Fork 780
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
Core: Create config.testIds to expose all available test ids #1424
base: main
Are you sure you want to change the base?
Conversation
Does this pre-hashing match the existing unique hashes during the run? I want to ensure these pre- and post-ids match up, and if there's any way to avoid hashing the same thing twice (we're pre-allocating, so maybe the latter can reuse that cache and not recalculate?). |
You are right, the new code did not account for that. I am going to abstract out the hashing collision management to a util and fix the code.
The hashing is incredibly fast, I had 10k tests and it returned the whole array of hashes immediately upon calling the getter. There's a storage tradeoff for caching for something that might not improve performance measurably. Also, QUnit.config.testIds only evaluates on demand for certain cases that need the testIds in advance. I would argue not to add caching complexities. |
bf39c18
to
8839e42
Compare
@smcclure15 Thanks for the review. Here's a screenshot of it working. |
@Krinkle and @trentmwillis appreciate some feedback here. |
86a9b27
to
9a1a788
Compare
What this does Currently each testId is calculated as each test is executed. Config.testIds pre-hashes all the loaded tests on demand and returns all testIds. How is this helpful Currently test runners can get access to test modules via config.modules and partition the modules based on some distribution strategy for running the tests in parallel. This is fine but it can lead to unbalanced distribution as some modules have a lot of tests and some modules are testing integration (longer running) test instead of unit test. Partitioning the tests by modules caused some instance of test runners to run way longer than others as some heavier running modules might be clumped together in the same instance. To mitigate and minimize the unbalanced test runs, the proposed solution is to partition based on each individual test with testId. The test runners can now access testIds beforehand and can distribute those ids with the testId url param. This will minimize the test execution time descrepencies between each individual instance of the test runners.
9a1a788
to
3c997f0
Compare
// Get the testIds for all tests | ||
// Simulates how test.js generates testIds by | ||
// iteratively pushing testNames to get uniqueTestName | ||
get testIds() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@trentmwillis I have a vague sense that using a getter here may bite us long-term in terms of confusion and flexibility. What's your take on this? Would making this a regular function be easier to understand/document/maintain long-term? Drawback is that it introduces a non-serialisable property to QUnit.config
. On the other hand, if config does get serialised, the value it would serialise to is likely wrong by the time it is consumed.
Maybe that's a reason to mount it somewhere else entirely, e.g. on currentModule
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just trying to get some traction here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@trentmwillis Any thoughts? I'd love to see this submitted to help tests run faster in parallel.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I've been busy working through other OSS tickets. From a brief glance, I think I'm inclined to agree that it doesn't belong on QUnit.config
. I'll review more thoroughly this Friday.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So after reviewing more thoroughly, I think this should just be a normal property that is an array.
The testId
for each test is already generated eagerly (i.e., when you call QUnit.test
). The information is already available within the (undocumented) QUnit.config.modules
property but would require some munging. So instead, we could set this up such that as each testId gets initialized, we push it into a new QUnit.config.testIds
property.
// Get the testIds for all tests | ||
// Simulates how test.js generates testIds by | ||
// iteratively pushing testNames to get uniqueTestName | ||
get testIds() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So after reviewing more thoroughly, I think this should just be a normal property that is an array.
The testId
for each test is already generated eagerly (i.e., when you call QUnit.test
). The information is already available within the (undocumented) QUnit.config.modules
property but would require some munging. So instead, we could set this up such that as each testId gets initialized, we push it into a new QUnit.config.testIds
property.
What this does
Currently each testId is calculated as each test is executed.
Config.testIds pre-hashes all the loaded tests on demand and returns all
testIds.
How is this helpful
Currently test runners can get access to test modules via config.modules
and partition the modules based on some distribution strategy for running
the tests in parallel. This is fine but it can lead to unbalanced
distribution as some modules have a lot of tests and some modules are
testing integration (longer running) test instead of unit test.
Partitioning the tests by modules caused some instance of test runners
to run way longer than others as some heavier running modules might be
clumped together in the same instance.
To mitigate and minimize the unbalanced test runs, the proposed solution
is to partition based on each individual test with testId. The test
runners can now access testIds beforehand and can distribute those ids
with the testId url param. This will minimize the test execution time
descrepencies between each individual instance of the test runners.