-
Notifications
You must be signed in to change notification settings - Fork 14
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
Feature/scaled testing #878
base: develop
Are you sure you want to change the base?
Conversation
cwms-data-api/src/test/java/cwms/cda/api/location/kind/GateChangeStressTest.java
Outdated
Show resolved
Hide resolved
} | ||
|
||
@Test | ||
void stress_test() { |
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.
Is there a better word than stress here? When I saw that immediately thought "parallel" not bulk data.
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.
should be scaled, sorry
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.
or scaling?
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.
Honestly that seems even more like something that should be parallel.
But yeah, that seems reasonable. Or at least as reasonable as the english language allows. Perhaps someone else will come up with something.
import static org.hamcrest.Matchers.is; | ||
import static org.junit.jupiter.api.Assertions.*; | ||
|
||
class GateChangeScaleTest extends BaseOutletDaoIT { |
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.
Disable the test, so it's not run in automation
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.
Don't disable the test. use @EnableIfProperty
(that might not be exact) so we don't need to change code to run the test when needed.
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.
Oh, that's a good idea, thank you. Didn't know that was a thing.
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.
FYI, I'd use Property over environment variable. I don't think we have the gradle build pass in the environment but we do pass in properties. There's a few other tests that I believe use this mechanism you can look at for an example.
Downloaded and tracked newer ts data with a lot more time points on it.
import static org.hamcrest.Matchers.is; | ||
import static org.junit.jupiter.api.Assertions.*; | ||
|
||
class GateChangeScaleTest extends BaseOutletDaoIT { |
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.
FYI, I'd use Property over environment variable. I don't think we have the gradle build pass in the environment but we do pass in properties. There's a few other tests that I believe use this mechanism you can look at for an example.
private static final ZonedDateTime TS_START = ZonedDateTime.of(2024, 5, 31, 0, 0, 0, 0, NumericalConstants.UTC_ZONEID); | ||
private static final Duration TS_DURATION = Duration.ofHours(120); | ||
|
||
private static final String CWBI = "https://cwms-data-test.cwbi.us"; |
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.
Tests should not be pointed at a specific URL.
- This is brittle, we cannot rely on cwbi-test services to be available; by definition even if in practice it usually is
- It limits who can do the testing.
In the setup section default this to the test environment URL; if it's important to point it to another location at times allow a property override.
LOGGER.atInfo().log("Retrieving TS data with page size: " + PAGE_SIZE); | ||
ZonedDateTime startTime = TS_START; | ||
|
||
for (int i = 1; i < info.getCurrentRepetition(); i++) { |
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.
Interesting use of repeated test to create a sliding window.
This PR contains a scaled integration test that initially stores gate changes for 1 year of 5 minute data for 18 gates. If this data already exists, it does not attempt to store the data, because it's a very time consuming process.
The actual test retrieves data for the entire range, but requests a row limit equal to 2 weeks of data and then logs the length of data and characters it received. This will help us identify if the JSON is too bulky.
Additionally, because the data storage is separated, the unit test will provide timing, and metrics are stored for the actual data retrieval.