Skip to content
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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

kltan
Copy link
Contributor

@kltan kltan commented Jan 8, 2020

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.

@smcclure15
Copy link
Member

Does this pre-hashing match the existing unique hashes during the run?
The existing hash mechanism modifies clashing test-names (white whitespace suffixes) to get unique IDs.

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?).

@kltan
Copy link
Contributor Author

kltan commented Jan 9, 2020

Does this pre-hashing match the existing unique hashes during the run?
The existing hash mechanism modifies clashing test-names (white whitespace suffixes) to get unique IDs.

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.

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?).

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.

@kltan
Copy link
Contributor Author

kltan commented Jan 10, 2020

@smcclure15 Thanks for the review. Here's a screenshot of it working.

image

@kltan
Copy link
Contributor Author

kltan commented Jan 10, 2020

@Krinkle and @trentmwillis appreciate some feedback here.

package-lock.json Show resolved Hide resolved
src/core/config.js Show resolved Hide resolved
@kltan kltan force-pushed the get-all-test-ids branch 3 times, most recently from 86a9b27 to 9a1a788 Compare January 10, 2020 21:01
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.
// Get the testIds for all tests
// Simulates how test.js generates testIds by
// iteratively pushing testNames to get uniqueTestName
get testIds() {
Copy link
Member

@Krinkle Krinkle Jan 13, 2020

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?

Copy link
Contributor Author

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.

Copy link
Contributor Author

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.

Copy link
Member

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.

Copy link
Member

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() {
Copy link
Member

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.

Base automatically changed from master to main March 18, 2021 18:59
@qunitjs qunitjs deleted a comment Sep 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants