Add GitHub Actions maven build and test for main branch

[srstsavage]

Description

• Adds GitHub Action in .github/workflows/maven.yml to build and test on pushes or pull_requests to main
• Moves test data from src/test/resources to test-data and adds test-data as a maven-surefire-plugin additionalClasspathElement
• Removes download-maven-plugin cacheDirectory overrides, moving cache directory to default .m2/respository/.cache/maven-download-plugin
• Fixes TopLevelHandlerTests which alters the value of EDStatic.angularDegreeUnitsSet, affecting subsequent tests (test order is indeterminate)
• Updates development/docker/Dockerfile (adds missing .mvn dir needed for errorprone, skips git-code-format hook installtion, and updates Java version to 21)

The test data move from src/test/resources to test-data was needed because many of the test cases require a specific data file to be the last file (latest mtime) in the matching set. maven-resources-plugin does not preserve mtime when copying test resources into target/test-classes, which was leading to files in test datasets having exactly the same mtime (the time at which maven-resources-plugin copied the files), and an indeterminate file was being selected as "latest". By using the test data in place via additionalClasspathElement, we preserve the mtimes and also avoid copying huge amounts of data into target/test-classes (win win).

Note: gha-test branch reference in .github/workflows/maven.yml can be deleted just before or after merge. It's just there to demonstrate working build/test.

Adds a simple GitHub Actions maven build and test for the main branch and pull requests against the main branch.

Type of change

• New feature (non-breaking change which adds functionality)

Checklist before requesting a review

• I have performed a self-review of my code
• My code follows the style guidelines of this project

[srstsavage]

There are four test failures in the GHA test, from these three test signatures:

EDDTableFromMultidimNcFilesTests#testTreatDimensionsAs
EDDTableFromInvalidCRAFilesTests#testBasic
EDDTableFromNcFilesTests#testOrderByMean2

Interestingly these all pass locally on my Linux machine with Java 21.

maven.test.redirectTestOutputToFile=true causes only failing test output to be logged to stdout, which makes the logs much more manageable.

download.cache.skip=true prevents two copies of the test data from existing in the test environment (the zipped cached version and the unzipped version). With both copies, the test server was exhausting disk space and needed pre-cleaning of unused tools bootstrapped by GitHub using https://github.com/marketplace/actions/free-disk-space-ubuntu. However, in the future we may want to let the Maven download plugin store the cache in its default of .m2/repository/.cache/maven-download-plugin so that it gets cached by the GitHub action runner between runs, in which case we'll probably need to perform some of the pre-cleaning to make room for both copies.

[ChrisJohnNOAA]

There are four test failures in the GHA test, from these three test signatures:

EDDTableFromMultidimNcFilesTests#testTreatDimensionsAs
EDDTableFromInvalidCRAFilesTests#testBasic
EDDTableFromNcFilesTests#testOrderByMean2

Interestingly these all pass locally on my Linux machine with Java 21.

maven.test.redirectTestOutputToFile=true causes only failing test output to be logged to stdout, which makes the logs much more manageable.

download.cache.skip=true prevents two copies of the test data from existing in the test environment (the zipped cached version and the unzipped version). With both copies, the test server was exhausting disk space and needed pre-cleaning of unused tools bootstrapped by GitHub using https://github.com/marketplace/actions/free-disk-space-ubuntu. However, in the future we may want to let the Maven download plugin store the cache in its default of .m2/repository/.cache/maven-download-plugin so that it gets cached by the GitHub action runner between runs, in which case we'll probably need to perform some of the pre-cleaning to make room for both copies.

The error in EDDTableFromNCFilesTest#testOrderByMean2 is interesting. The file has the longitude as 350. On all other machines it gets convered here:

erddap/WEB-INF/classes/gov/noaa/pfel/erddap/dataset/TableWriterOrderByMean.java

Line 374 in c492815

meansTable.setDoubleData(col, row, accum == null ? Double.NaN : accum.getMean());

to be -10. Which is because the variable's attributes are degree_east. In order to not be converted, I'd expect the units to need to be in this list:
public static final String DEFAULT_ANGULAR_DEGREE_TRUE_UNITS =
"degreesT,degrees_T,degrees_Tangular_degree,degrees_true," + "degreeT,degree_T,degree_true";

It's possible the GitHub action is using a different version of the netcdf library that changes the units of that column, but that'd be quite surprising to me.

[srstsavage]

The EDDTableFromNCFilesTests#testOrderByMean2 failure was due to TopLevelHandlersTests changing the value of EDStatic.angularDegreeUnitsSet. The order of tests is non-determinate and apparently is different on the GitHub action runner.

https://github.com/ERDDAP/erddap/blob/main/src/test/resources/datasets/topLevelHandlerTest.xml#L7

Fixed that here:

srstsavage@3ea5ec9

I'm reviewing the rest of the failures in the context of your PR to try to understand the source of failure better, will keep this PR a draft until they're resolved and the commits are cleaned up.

[srstsavage]

@ChrisJohnNOAA At long last this should be ready to go. I updated the description with notes about additional changes needed to make the tests pass (most importantly moving test data from src/test/resources to test-data). I expect that this might eliminate some of the test flakiness that we've been working around with adaptive expected strings.

[srstsavage]

One other note: the total cache GitHub actions cache size for a repo is 10GB. The size of the cached data payload (everything in ~/.m2/repository which includes all dependency jars and download-maven-plugin cache) is just over 5GB. The cache is keyed by a hash of pom.xml. When the cache is too full, GitHub deletes the oldest cache payload to make room. What this means is that we'll only have one cached payload (or two if the size drops under 5GB), but that should be fine because a newer cache payload will be built whenever the pom changes and subsequent builds using that pom will use the new cache.

[ChrisJohnNOAA]

@ChrisJohnNOAA At long last this should be ready to go. I updated the description with notes about additional changes needed to make the tests pass (most importantly moving test data from src/test/resources to test-data). I expect that this might eliminate some of the test flakiness that we've been working around with adaptive expected strings.

How is using test-data different that it will eliminate the flakiness?

[ChrisJohnNOAA]

Won't removing the cache directory cause the files to need to be downloaded repeatedly?

[srstsavage]

Nope, removing cacheDirectory will cause the default of ~/.m2/repository/.cache/maven-download-plugin to be used for the cache, and since that's inside of ~/.m2/repository it will be included in the GitHub action maven cache.

https://github.com/maven-download-plugin/maven-download-plugin/blob/0d0fd979794a5b0c941ea5175a891cc8f22c432a/src/main/java/com/googlecode/download/maven/plugin/internal/WGetMojo.java#L191

https://github.com/actions/setup-java/blob/main/src/cache.ts#L26

[ChrisJohnNOAA]

I believe developers need to clear the /data/ directory because of the change to src/test/resources/ -> test-data. I had a lot of test failures initially trying to run tests on these changes.

[srstsavage]

Ah yeah, good point. Do you think we need to handle deletion of the previous src/test/resources/{data,largeFiles,largePoints,largeSatellite} through an ant task? Or exclude those directories from testResources? Or just try to communicate to developers to clear out those files?

[srstsavage]

How is using test-data different that it will eliminate the flakiness?

This is more of a speculation and I haven't tested, but it should make any test more stable where the expected output derives from the most recent file (largest mtime) in the dataset and there's variability in the tested fields through the dataset files. If there are cases where we previously were adding some variability to the expected test strings to accommodate this, we may have consistent results now.

(Also please see my justification for the test data move in the edited PR description above if you haven't already 😁)

[srstsavage]

Added a check for test data directories in src/test/resources using the enforcer plugin, seems to work well.

a42bc99

[ERROR] Failed to execute goal org.apache.maven.plugins:maven-enforcer-plugin:3.5.0:enforce (no-test-data-in-src-test-resources) on project ERDDAP: 
[ERROR] Rule 0: org.apache.maven.enforcer.rules.files.RequireFilesDontExist failed with message:
[ERROR] Test data directories should be removed from src/test/resources
[ERROR] Some files should not exist:
[ERROR] /home/shane/src/erddap/src/test/resources/data
[ERROR] /home/shane/src/erddap/src/test/resources/largeFiles
[ERROR] /home/shane/src/erddap/src/test/resources/largePoints
[ERROR] /home/shane/src/erddap/src/test/resources/largeSatellite

[ChrisJohnNOAA]

Added a check for test data directories in src/test/resources using the enforcer plugin, seems to work well.

a42bc99

[ERROR] Failed to execute goal org.apache.maven.plugins:maven-enforcer-plugin:3.5.0:enforce (no-test-data-in-src-test-resources) on project ERDDAP: 
[ERROR] Rule 0: org.apache.maven.enforcer.rules.files.RequireFilesDontExist failed with message:
[ERROR] Test data directories should be removed from src/test/resources
[ERROR] Some files should not exist:
[ERROR] /home/shane/src/erddap/src/test/resources/data
[ERROR] /home/shane/src/erddap/src/test/resources/largeFiles
[ERROR] /home/shane/src/erddap/src/test/resources/largePoints
[ERROR] /home/shane/src/erddap/src/test/resources/largeSatellite

I was actually talking about the erddap/data/ directory that is used to cache data for the server (one of the things that's cached is file paths to where the data was- I've been wondering if it should more gracefully handle moves like this).

Related to this. I think the paths that are generated by EDDTestDataset need to get updated with the move to test-data. I can look more into that tomorrow.

[srstsavage]

I was actually talking about the erddap/data/ directory that is used to cache data for the server (one of the things that's cached is file paths to where the data was- I've been wondering if it should more gracefully handle moves like this).

Ah, gotcha. Seems like we'd want the bigParentDirectory for testing to be transient, right? Should we move that into ./target so it can be cleaned up between runs with mvn clean? This two line change does that and seems to work (all tests pass using mvn clean test anyway).

diff --git a/development/test/setup.xml b/development/test/setup.xml
index e2867023..473da005 100644
--- a/development/test/setup.xml
+++ b/development/test/setup.xml
@@ -19,7 +19,7 @@ Subdirectories that will be created by ERDDAP are:
 The ERDDAP log file (log.txt) will be put in this directory.
 At ERD, we use '/u00/cwatch/erddap/' for releases.
 -->
-<bigParentDirectory>data</bigParentDirectory>
+<bigParentDirectory>target/test-erddapData</bigParentDirectory>
 
 <!-- baseUrl is the start of the public url, to which "/erddap" 
 is appended. For example:
diff --git a/pom.xml b/pom.xml
index cb12fa31..41472477 100644
--- a/pom.xml
+++ b/pom.xml
@@ -235,7 +235,7 @@
                         </goals>
                         <configuration>
                             <target>
-                                <mkdir dir="${project.basedir}/data"/>
+                                <mkdir dir="${project.build.directory}/test-erddapData"/>
                             </target>
                         </configuration>
                     </execution>

Related to this. I think the paths that are generated by EDDTestDataset need to get updated with the move to test-data. I can look more into that tomorrow.

It looks like all of the places that generate fileDir in EDDTestDataset use getResource(), and since we put ./test-data on the test classpath using additionalClasspathElement things seem to work normally after the move.

[ChrisJohnNOAA]

I made a pull request against your branch to allow the integration tests (mvn verify) to work with the moved test data: srstsavage#5

[srstsavage]

Thanks, merged that! Any thoughts on if we should be running mvn verify in the automated build instead of mvn test? mvn test/surefire completes in 5 - 10 minutes on the runner and locally for me. mvn verify/failsure takes over an hour locally. Maybe we run mvn test for PRs and also run a scheduled daily or weekly mvn verify? Or since it's running on a remote runner maybe it's fine to have the PR build/test take over an hour? Or maybe even run both so that we have quick feedback on failing surefire tests but also eventual reassurance of passing failsafe tests?

[ChrisJohnNOAA]

Thanks, merged that! Any thoughts on if we should be running mvn verify in the automated build instead of mvn test? mvn test/surefire completes in 5 - 10 minutes on the runner and locally for me. mvn verify/failsure takes over an hour locally. Maybe we run mvn test for PRs and also run a scheduled daily or weekly mvn verify? Or since it's running on a remote runner maybe it's fine to have the PR build/test take over an hour? Or maybe even run both so that we have quick feedback on failing surefire tests but also eventual reassurance of passing failsafe tests?

I think we should have both running on pull requests (assuming GHA time is free as an open source project, which I believe it is). Just running mvn verify does also run the (mvn test) tests first, so we could likely do it with the one command. I think it's better to catch test breakages early rather than needing to hunt for what broke something (if only running once a week). As for the quick feedback of just the mvn test on a pull, we can have that separate if it's useful. I'd hope for most pull requests the quick (mvn test) tests have been run locally so there should be no surprises on the server.

[ChrisJohnNOAA]

Looks good. As we were discussing we could change the command to verify instead of test (or add an additional command for verify), but this will be a great step as is.