search

NOAA-OWP / wres

Code and scripts for the Water Resources Evaluation Service
Other
2 stars 1 forks source link

Investigate why commit action did not run all unit tests #309

Closed epag closed 2 months ago

epag commented 2 months ago

296 passed our github PR gates but caused unit tests to fail. Investigate and add additional testing to the action if needed

epag commented 2 months ago

Ok, I did some investigation:

What we currently have in our build script (And what we had in Jenkins):

https://github.com/NOAA-OWP/wres/blob/7c92b8009832d730cfab20840569c3da960d14fa/.github/workflows/commitChecks.yml#L27-L30

This does not actually run the unit tests for worker/tasker and is why we didn't see the failure introduced in #296

Unsure the reasoning we were doing thing in this way initially, but I have seen that ./gradlew test does seem to kick off the wres-worker:test

I see two paths forward:

epag commented 2 months ago

Currently our code check action runs on push, pull request, merge, etc (Basically every action related to code). Ideally that action would be very light weight.

I think my current thought is the following:

  1. Create a new action that runs on every push that simply runs gradlew test or maybe gradlew build (Will take longer but I believe this will test more)
  2. Also add the test/build command into the current action we have that has the gate for a PR to go through

This way we run the leaner job more often/allows people to use a commit to ad-hawk test their code, but we for sure want to sacrifice some time to test everything before allowing someone to push their code. Based on the above though it seems like we haven't been testing working/tasker code automatically for awhile now

epag commented 2 months ago

Okay, I tried adding ./gradlew build to a fork I made and I am getting this error, it seems related to this test:

2024-09-09T13:32:50.8724929Z   Test testReadRequestsDateRangeWithMinutesAndSeconds() FAILED
2024-09-09T13:32:50.8726002Z 
2024-09-09T13:32:50.8726508Z   java.lang.AssertionError: Request not found exactly once, expected:<{
2024-09-09T13:32:50.8727513Z     "method" : "GET",
2024-09-09T13:32:50.8728290Z     "path" : "/api/v1/nwm/ops/analysis_assim/streamflow/nwm_feature_id/8588002/",
2024-09-09T13:32:50.8729200Z     "queryStringParameters" : {
2024-09-09T13:32:50.8729885Z       "reference_time" : [ "(20240901T000000Z,20240903T002759Z]" ],
2024-09-09T13:32:50.8730658Z       "proj" : [ "UNKNOWN_PROJECT_USING_WRES" ],
2024-09-09T13:32:50.8731330Z       "forecast_type" : [ "deterministic" ]
2024-09-09T13:32:50.8731913Z     }
2024-09-09T13:32:50.8732252Z   }> but was:<{
2024-09-09T13:32:50.8732632Z     "headers" : {
2024-09-09T13:32:50.8733148Z       "content-length" : [ "0" ],
2024-09-09T13:32:50.8733980Z       "User-Agent" : [ "wres-http/20240906-7c92b80" ],
2024-09-09T13:32:50.8734719Z       "Connection" : [ "Keep-Alive" ],
2024-09-09T13:32:50.8735296Z       "Host" : [ "localhost:38343" ],
2024-09-09T13:32:50.8735907Z       "Accept-Encoding" : [ "gzip" ]
2024-09-09T13:32:50.8736420Z     },
2024-09-09T13:32:50.8736886Z     "keepAlive" : true,
2024-09-09T13:32:50.8737410Z     "method" : "GET",
2024-09-09T13:32:50.8738154Z     "path" : "/api/v1/nwm/ops/analysis_assim/streamflow/nwm_feature_id/8588002/",
2024-09-09T13:32:50.8739037Z     "queryStringParameters" : {
2024-09-09T13:32:50.8739663Z       "reference_time" : [ "(20240901T00Z,20240908T00Z]" ],
2024-09-09T13:32:50.8740379Z       "proj" : [ "UNKNOWN_PROJECT_USING_WRES" ],
2024-09-09T13:32:50.8741023Z       "forecast_type" : [ "deterministic" ]
2024-09-09T13:32:50.8741561Z     },
2024-09-09T13:32:50.8741903Z     "secure" : false
2024-09-09T13:32:50.8742337Z   }>
2024-09-09T13:32:50.8743827Z       at wres.reading.wrds.nwm.WrdsNwmReaderTest.testReadRequestsDateRangeWithMinutesAndSeconds(WrdsNwmReaderTest.java:648)
2024-09-09T13:32:50.8745241Z 
2024-09-09T13:32:50.8745441Z 
2024-09-09T13:32:50.8745927Z wres.reading.wrds.thresholds.ThresholdExtractorTest
2024-09-09T13:32:50.8746563Z

Seems like a header was added so the assert failed.

epag commented 2 months ago

Actually pulling a fresh checkout is failing build as well with the same test. Going to fix this first 

java.lang.AssertionError: Request not found exactly once, expected:<{
  "method" : "GET",
  "path" : "/api/v1/nwm/ops/analysis_assim/streamflow/nwm_feature_id/8588002/",
  "queryStringParameters" : {
    "reference_time" : [ "(20240901T000000Z,20240903T002759Z]" ],
    "proj" : [ "UNKNOWN_PROJECT_USING_WRES" ],
    "forecast_type" : [ "deterministic" ]
  }
}> but was:<{
  "headers" : {
    "content-length" : [ "0" ],
    "User-Agent" : [ "wres-http/20240829-cae067a" ],
    "Connection" : [ "Keep-Alive" ],
    "Host" : [ "localhost:49337" ],
    "Accept-Encoding" : [ "gzip" ]
  },
  "keepAlive" : true,
  "method" : "GET",
  "path" : "/api/v1/nwm/ops/analysis_assim/streamflow/nwm_feature_id/8588002/",
  "queryStringParameters" : {
    "reference_time" : [ "(20240901T00Z,20240908T00Z]" ],
    "proj" : [ "UNKNOWN_PROJECT_USING_WRES" ],
    "forecast_type" : [ "deterministic" ]
  },
  "secure" : false
}>
    at org.mockserver.client.MockServerClient.verify(MockServerClient.java:1030)
epag commented 2 months ago

Seems like this recent commit that added the minutes and seconds change is the issue:

https://github.com/NOAA-OWP/wres/commit/2d3ffcb86d4a54b4b9049b58b658ee633efe544a

epag commented 2 months ago

Looks like just an issue with the header information though so ill fix this after my appointment which I gotta run to quick

james-d-brown commented 2 months ago

Evan,

Not working but just spotted this and I will save you a potentially significant and confusing headache :)

It has nothing to do with the header. The unit test was passing before Sunday, but it has an implicit timebomb, which is why it is now failing. In reality, this unit test should be yanked and part of the above commit should be yanked too because I have subsequently realized that, while we should read wrds responses that contain minutes and seconds (the purpose of ticket #292), we should NOT request wrds data with periods that end with minutes and seconds. Why? Because we aim to promote re-use of data in the database by imposing predictable request periods on the time-series data. For this reason, I will back out this behavior when I return, but not the full commit, only part of it.

For now, you can either mark that unit test @Disabled or yank it entirely, whichever you prefer.

Beyond that, you should feel free to deploy as normal, when ready, as this new behavior should have no adverse consequences other than the failing unit test, which had a bad assumption that created a timebomb (largely because the dates within the test are close to the current date, but now the current date has ticked forward beyond an assumption made by the underlying code, which builds weekly request periods from Sunday to Sunday).

epag commented 2 months ago

Ack, thanks for the heads up!

epag commented 2 months ago

I just went ahead and disabled that test for the time being and ill leave it to James to determine what all he wants to do with it once he returns

epag commented 2 months ago

Alright, adding the ./gradlew build step reveals the issue that kicked off this ticket. Unsure why in Jenkins we did this originally instead of just a build ./gradlew distZip testCodeCoverageReport javadoc if any of you know.

Regardless I went ahead and added the full build step into the action. We can discuss in our next dev meeting how/if we want to optimize how these actions run