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.

Sign up for GitHub

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

Jump to bottom

Add a time-based STF system test #384

Merged
merged 1 commit into from
Dec 16, 2020
Merged

Add a time-based STF system test #384

merged 1 commit into from
Dec 16, 2020

Conversation

Mesbah-Alam
Copy link
Contributor

@Mesbah-Alam Mesbah-Alam commented Dec 10, 2020 •
edited
Loading

  • Adds a time-based STF system test

Related to adoptium/aqa-tests#2104

FYI @lumpfish

Signed-off-by: Mesbah_Alam@ca.ibm.com Mesbah_Alam@ca.ibm.com

@Mesbah-Alam Mesbah-Alam changed the title Add timing based load test Add a time-based STF system test Dec 10, 2020
@Mesbah-Alam
Copy link
Contributor Author

Mesbah-Alam commented Dec 10, 2020 •
edited
Loading

Test:
zLinux : InternalGrinder/12632
Windows: InternalGrinder/12633

@Mesbah-Alam Mesbah-Alam mentioned this pull request Dec 10, 2020
Merged
@Mesbah-Alam Mesbah-Alam self-assigned this Dec 10, 2020
@Mesbah-Alam Mesbah-Alam added this to the December 2020 milestone Dec 10, 2020
llxia
llxia reviewed Dec 12, 2020
View reviewed changes
openjdk.test.load/src/test.load/net/adoptopenjdk/stf/TimedLoadTest.java Outdated
String timelimitInMins;
int testCountMultiplier;
public void help(HelpTextGenerator help) throws StfException {
help.outputSection("MixedLoadTest runs workloads of math,mauve,nio,lang,concurrent unit tests");
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this TimedLoadTest or MixedLoadTest ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch! That was a copy paste error. I've updated the help message.

@llxia llxia requested a review from lumpfish December 12, 2020 18:42
@lumpfish
Copy link
Contributor

I do not think 'TimedLoadTest' is what this test should be called - assuming you might want to create other workload tests which are timed based what would you call the next one?

As far as I can see, this is actually just a variant of MixedLoadTest running for a period of time rather rather than a fixed number of tests. So why not just modify MixedLoadTest to accept the timeLimit argument? If the argument is not received, then the test would behave as is does now; if the argument is present it would be used as the timeLimit and the number of tests to be run increased so that they do not complete before the timelimit expires.

Also, why only allow the user to specify the timeLimit in minutes? The STF -timeLimit option actually uses a 5s / 5m / 5h syntax so it makes sense for the -test-args argument to use the same syntax.

If that were done then a playlist or test makefile could have several targets for the same workload such as:

MixedLoadTest:
	stf.pl -test=MixedLoadTest
MixedLoadTest_5secs:
	stf.pl -test=MixedLoadTest -test-args="timeLimit=5s"
MixedLoadTest_5mins:
	stf.pl -test=MixedLoadTest -test-args="timeLimit=5m"
MixedLoadTest_5hrs:
	stf.pl -test=MixedLoadTest -test-args="timeLimit=5h"

lumpfish
lumpfish suggested changes Dec 14, 2020
View reviewed changes
Copy link
Contributor

@lumpfish lumpfish left a comment

Choose a reason for hiding this comment

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

See comment in PR Conversation regarding requested changes.

@Mesbah-Alam
Copy link
Contributor Author

@lumpfish - Thanks for the review comments. The following changes have been made based on them:

  1. Removed TimedLoadTest altogether.
  2. Updated MixedLoadTest to handle the case where the input parameters are passed in - in which case it will set the time cap, otherwise the default test load will be run
  3. Updated the playlsit to pass in value like 5m as accepted by STF -timeLimit option.

Tested time-bsased case here: https://ci.adoptopenjdk.net/view/Test_grinder/job/Grinder/5483/
Tested default case here: https://ci.adoptopenjdk.net/view/Test_grinder/job/Grinder/5486/console

@Mesbah-Alam Mesbah-Alam force-pushed the add-5mins-minimix branch 3 times, most recently from 18df835 to b996820 Compare December 15, 2020 16:55
@Mesbah-Alam
Copy link
Contributor Author

Latest changes were tested here: https://ci.adoptopenjdk.net/view/Test_grinder/job/Grinder/5489/

lumpfish
lumpfish approved these changes Dec 16, 2020
View reviewed changes
Copy link
Contributor

@lumpfish lumpfish left a comment

Choose a reason for hiding this comment

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

Looks good. Please "Squash and Merge" when you're ready.

@Mesbah-Alam Mesbah-Alam merged commit 7ae1800 into adoptium:master Dec 16, 2020
@Mesbah-Alam Mesbah-Alam deleted the add-5mins-minimix branch December 29, 2020 19:42
fjeremic
fjeremic reviewed Jan 11, 2021
View reviewed changes
openjdk.test.load/src/test.load/net/adoptopenjdk/stf/MixedLoadTest.java
Comment on lines +91 to +93
if (isTimeBasedLoadTest) {
loadTestInvocation = loadTestInvocation
.setSuiteNumTests(totalTests * 80000000 * testCountMultiplier); // If it's a time based test, run a very large load
Copy link

Choose a reason for hiding this comment

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

@Mesbah-Alam shouldn't we just set this to INT_MAX or LONG_MAX or whatever the maximum representable integer is rather than using a "very large load" with a magic 80,000,000 constant? The tests will be terminated after the alloted time anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for your suggestion. I will try this out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fjeremic fjeremic fjeremic left review comments

@llxia llxia llxia left review comments

@lumpfish lumpfish lumpfish approved these changes

Assignees

@Mesbah-Alam Mesbah-Alam

Labels
enhancement
Projects
None yet
Milestone
December 2020
Development

Successfully merging this pull request may close these issues.
4 participants
@Mesbah-Alam @lumpfish @fjeremic @llxia