-
Notifications
You must be signed in to change notification settings - Fork 79
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
Allow local runs of test targets to parametrize parallelism and heap size. #1943
Allow local runs of test targets to parametrize parallelism and heap size. #1943
Conversation
jcferretti
commented
Feb 5, 2022
•
edited
edited
This shows timings per tests: |
Scan for |
Got testParallel down to 4 minutes with the latest changes:
|
Scan for |
…rking before, which was hidden by another mistake.
I can run the three targets
(note the parallel an other flags above do not apply to |
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.
No major holdups, a few small things to change. As far as I could tell, the existing CI should be running exactly the same. I can't vouch that there aren't logical or race-condition issues running these tests in a more parallel nature locally. I think there is a lot of continued work we'll want / need to do as we continue to improve our test-ability.
final int steps = TstUtils.SHORT_TESTS ? 20 : 100; | ||
|
||
for (int step = 0; step < steps; ++step) { |
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.
From an architectural standpoint, I think everything that relies on TstUtils.SHORT_TESTS
(or more generally, some arbitrary number of iterations/seeds for testing) is a "code smell", at least from the perspective of "unit tests" that guard PRs from merging. It may just be around the nomenclature we use, but my future wishlist would include rebrading and reconfiguring how we think about these types of "tests".
That said, I understand the practical nature of why it is the way it is, and that we want it to be faster - and I'm not proposing you fix our architectural shortcomings. At the very least, I'm happy that we'll have hard references to a lot of these places now that SHORT_TESTS is being introduced.
final int nSeeds = scaleToDesiredTestLength(20); | ||
final int tableSizeSteps = scaleToDesiredTestLength(1000); | ||
final int maxTableSize = 10000; | ||
final int tableSizeStep = maxTableSize / tableSizeSteps; |
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.
I think "Step" is a misnomer - in my mind, it implies equally-spaced additions. I think "Mult" may be better? (Note: same comment in a lot of other files too).
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.
I actually did not notice int this case this was a factor instead of an additive step; thanks.
test-configs/dh-short-tests.prop
Outdated
@@ -0,0 +1,106 @@ | |||
# Properties for deephaven unit tests |
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.
As mentioned earlier, this file may be redundant depending on setup.
Run after last batch of changes:
|