ERDDAP build and unit testing changes

[clifford-harms]

I wanted to kick off a discussion about implementing some changes to Erddap to see what you guys think before I do the work and fire off some pull requests.

What I would like to do:

• alter Erddap build and source code structure to match maven standard structure.
• Make changes to Erddap source to allow bootstrapping the Erddap servlet within a unit test

Why
My organization would like to begin contributing to Erddap, and it looks like there are others interested as well. The ability to easily execute unit tests, either in suite or individually, is essential to maintaining a quality baseline with multiple contributors. Tests that are difficult to execute or require extensive setup don't get executed often.

Proposed Changes

Move to a maven standard structure for a java web application, as below:

/
|_ src/
      |_ main/
      |        |_ webapp/
      |        |             |_ WEB-INF/
      |        |             |_ html, other folders, etc
      |        |_ java/
      |        |_ resources/
      |_ test
      |        |_ java/
      |        |_ resources/
|_ lib/  

The lib directory would be for installation of problematic jars (i.e. netcdfALL) that cannot be reliably retained from remote repositories. All non-java source files would be moved to an identical package structure in src/main/resources.

The next step (a separate merge request) would be to make Erddap instantiable in unit tests, so that in addition to basic unit tests, tests can be created against a n actual running erddap servlet. This would involve:

• Encapsulating the EDStatic.java static initialization into a static EDStatic.init() method. This method can be invoked to initialize erddap when desired, instead of it happening at class definition. So you could initialize Erddap in a unit test, or a web context listener.
• Adding a web context listener to Erddap that invokes the EDStatic.init() method
• Changing the current filesystem lookup for classpath resources to classpath resource lookups. I'm not sure why this is done via the filesystem, but it is not necessary and makes Erddap extremely sensitive to where/how it is deployed.
• Adding a test support class that boostraps erddap and makes functionality available to unit tests.

Small subsets of test data can be version controlled in test/resources (compressed) in support of unit tests. Inclusions to this test dataset do have to be made carefully to prevent size from becoming an issue.

The end result is an ERDDAP that builds easily, can be extensively unit tested, with no changes to how organizations deploy/use ERDDAP.

I have done all of this successfully against ERDDAP v2.18 for my organization, and would be happy to port it to your main HEAD, but I wanted to make sure there was no immediate opposition to these types of changes prior to doing the work to create a merge request.

[ChrisJohnNOAA]

I fully agree that making the tests easy for contributors to run is critical to supporting multiple developers and that the current test set up is too complicated for many of the tests.

Generally I prefer pull requests focused on changing one thing at a time in order to keep them smaller. To that end, I’d prefer updates to the tests first to be more easily runnable (this probably includes the change from filesystem to classpath lookup). After the tests are executing successfully, I’d be in support of other changes. One thing to keep in mind is that the current TestAll file is a critical part of building the distributed war file.

We should also clarify what changes to the tests you would be making. Does this include using JUnit? Will test code be moved out of class files into the test/java directory? Will tests that talk to a local server be changed to use the new servlet? Will tests that talk to an external ERDDAP be updated to use the servlet? Will tests that talk to external servers (like THREDDS) be updated to use mock data?

Given how ERDDAP is currently reliant on filesystem lookup and the TestAll file, I want to do manual verification the war file built after these changes has no user visible differences (except being smaller if all of the test code/data is no longer included).

Lastly, I believe folks at Axiom had similar thoughts to your proposal. I’d like to have them chime in on this proposal as well. I think they may have had a slightly different folder structure as their desired end result.

[clifford-harms]

Apologies for the following novella.

Generally I prefer pull requests focused on changing one thing at a time in order to keep them smaller.

Agree 100%

We should also clarify what changes to the tests you would be making. Does this include using JUnit?

Absolutely

Will test code be moved out of class files into the test/java directory?

Yes, but moving the existing (and substantial) tests does not have to happen initially, or even at all. The old tests will be right where they are now, and can be executed as they are currently. The idea is to have a structure to a.) add new tests to and b.) migrate existing tests to when/if it makes sense or someone wants to take that on.

Will tests that talk to a local server be changed to use the new servlet?

Tests that talk to the local server would use THE erddap servlet. The only difference between that servlet running in tomcat and the servlet running in Junit would be who provides the HttpServletRequest/Response objects (Tomcat vs a mocked HttpServletRequest/Response).
Here is code example from an actual unit test I have running now, sans the actual deserialization/checks of the returned json data
for brevity:

  /**
    * TestSupport configures and instantiates the Erddap servlet,
    * and registers test data.  Note that there is no server process running
    * for this
 * */
  TestSupport testSupport = TestSupport.builder()
        .usingDefaultTestData()
        .build();

  //these mocks are from spring-test, there are other libraries to choose from
  MockHttpServletRequest request = new MockHttpServletRequest();
  MockHttpServletResponse response = new MockHttpServletResponse();

  request.setRequestURI("/erddap/griddap/hycom_climo_subset.json");
  String query= "water_temp%5B(1993-05-01T00:00:00Z)%5D%5B(0.0)%5D%5B(26.399999618530273):" 
         "(27.360000610351562)%5D%5B(-77.760009765625):(-76.8800048828125)%5D";
  request.setQueryString(query);
   
  //erddap() returns a reference to the Erddap servlet
  testSupport.erddap().doGet(req,resp);

  assertEquals(200,resp.getStatus());
  assertEquals("application/json;charset=UTF-8",resp.getHeader("Content-Type"));
  String json = response.getContentAsString();
  //check the validity of the returned json

You could of course write tests directly against other Erddap components (like EDDGrid/EDDTable, etc.) if you do not need to test the entirety of the transaction through the servlet, depending on the use case. This type of simplicity in introducing new tests is what I am proposing, as well as simplicity in executing tests:

//run all tests
mvn test

//run a specific test
mvn test -Dtest=SomeTestClass

This also allows easy generation of test reports, integration with other modern tooling (IDE's, CI/CD tools, SAST/DAST tools and the like), which are becoming prevalent and in some cases required before deploying something operationally.

Will tests that talk to an external ERDDAP be updated to use the servlet? Will tests that talk to external servers (like THREDDS) be updated to use mock data?

I have not written any tests yet that reach out to external servers, I will do so and see how that fares. I (personally) find it easy to get carried away / overly complicated with mocking, and prefer to test the real thing whenever possible, the HttpServletRequest / Response being a notable exception because their behavior is well governed by specification. There is nothing that prevents the Erddap servlet from making a request to external servers if the unit test requires it though, as long as a viable network path exists. I think a good practice is to check for the availability of that external service in the test, and skip the test with a warning if it the service is not available (Junit does this with Assume) instead of failing the test outright.

Given how ERDDAP is currently reliant on filesystem lookup and the TestAll file, I want to do manual verification the war file built after these changes has no user visible differences (except being smaller if all of the test code/data is no longer included).

Understood, and I plan to thoroughly test myself of course. I do think that the changes needed to bootstrap erddap independent of an application server are smaller than you might expect (a few dozen lines all told?). The majority of the work is done by the TestSupport class, which makes test data available, but is self contained and does not affect the Erddap deployable in any way.

I am having trouble describing the extent of the changes needed, which I believe to be small, so the best way to do this would be to submit an actual merge request, which I will do as it seems there is at least some tentative interest. I won't bother with moving the source code to the src/{main,test,webapp} structure, as it is not strictly necessary outside of requiring some maven configuration hurdles.

[BobSimons]

In the end, I leave decisions about this to Chris. I know my preferences are different than the younger generation of developers. But my questions and comments are:

1. Can you please explain to me: ERDDAP is a web application built to run inside an application like Tomcat. So many of the tests rely upon a running ERDDAP, within a Tomcat. ERDDAP installed in Tomcat is in a different directory structure than it ERDDAP in Maven. What is the solution for this need to have ERDDAP in 2 different file structures? Isn't that an inherent problem with only kludges for solutions?

2. Until AWS Java SDK required the use of Maven, I always avoided adding dependencies on third party development tools, The reason was: different developers prefer (or insist on!) different developer tools so requiring one will almost always lead to strong complaints from other developers. I understand that Maven is now widely used, but it may not always be so. Things change. A lot of Java developers did everything in Eclipse. To work on certain projects, you had to use Eclipse. Times change. I thought it was a bad idea to become dependent on one (or several) specific tools.

3. After John Caron left Unidata, the netcdf-java group set about modernizing the netcdf-java code base. The result was 5+ years of sideways development (few new features, tons of changes to the existing API thereby breaking all code that used netcdf-java, new bugs, bug fixes, etc.). I have a major concern that the same could happen to ERDDAP. It would be easy to spend 5 years, revamping the tests, switching to Maven and JUnit, etc. The current system works. The current system is clean and simple. There is a place for everything and everything is in its place. There are no dependencies on external tools (with the partial exception of Maven, but a developer doesn't have to use Maven to work on ERDDAP).

4. I don't see why you need to make changes to ERDDAP's system for tests, etc. The current system works. It is clear how to add new tests and how to run them. Isn't your time better spent adding the new features you want to add?

[clifford-harms]

Bob! I've been seeing your name quite a bit during my time with Erddap!

I made a rather lengthy response to Chris, so I'll try not to repeat myself. I do agree with you on refactoring - it is rarely successful or worth the effort, and it is not my intent at all. I am familiar with the NetCDF5 scenario, and have personally experienced other refactoring disasters as well. I think a merge request will make the extent of the changes I am proposing more clear then I can express in this discussion, so I am going to work on that. I do have to (respectfully) disagree on the simplicity of testing in erddap (not ERDDAP itself). I have found it to be somewhat of a barrier, particularly with new / junior developers, both in the execution and extraction of meaningful information from the test execution. In addition, the current testing just doesn't work with modern tooling that many folks are beginning to require (Coverage reports, static analysis, etc etc.).

I do appreciate you and Chris taking the time to comment/interact with me on this, and will work on that merge request where you guys can pass judgement.

[ChrisJohnNOAA]

The current system for tests works on your machine, Bob. It has an abundance of hardcoded file paths based on where the files are stored on your computer. Many of these files are not available with ERDDAP (or ERDDAP test data), and in some cases it is unclear where to get them. This makes a new developer getting set up a very painful and time consuming process. In addition there are many tests that fail due to external dependency problems (like the THREDDS tests), as well as a number of error logs that are expected. It also means that new developers are unlikely to be able successfully run the existing tests and even a successful run has a lot of output in the error log. I recently had to re-setup my environment for ERDDAP development and the first time I ran the tests I had approximately 30,000 lines of output in the error log.

Concerns with the current tests for ERDDAP has been brought up to me by multiple groups. This isn’t one person’s opinion.

Besides improving the experience for a new developer (which will help us get contributions to ERDDAP code from more people), using JUnit and maven for testing will enable features and tools to help ensure the quality of pull requests and ERDDAP. This includes being able to run the tests automatically for each pull request, use analyzers to verify test coverage of new and existing code, and more.

Not all of ERDDAP’s tests make sense to have in JUnit, there are many different kinds of tests. It does make sense to have end to end integration testing of an ERDDAP running in Tomcat, however there are many tests that gain no benefit from running within Tomcat. Testing the Tomcat integration is worthwhile and there will be tests for that, but having more tests runnable in JUnit with no setup needed outside of the code will have major benefits.

I believe that adopting JUnit (and having the tests runnable in Maven- though to be specific Maven is a way to run the tests it is not required for it), will provide very real benefits for ERDDAP.

[BobSimons]

@ChrisJohnNOAA, thanks for those comments. I see your points.

[BobSimons]

I should have also said regarding the hard coded paths: I was responsible for developing ERDDAP and being the administrator of an ERDDAP. I set up many tiny test datasets in a data structure on my computer that paralleled the coastwatch structure so that the some/much of the datasets.xml that I used for ERDDAP datasets could also be used for testing ERDDAP. Given that that need/benefit is now gone, it should be easy for you to move those tiny datasets to the erddapTest directory (or similar in Maven or wherever) so that the files are version controlled and available to others.

[srstsavage]

First, I wholeheartedly agree that moving to a (more) standard directory structure and build methodology will lower the barrier of entry for new developers, make the ERDDAP codebase more robust and portable, increase confidence that changes to the code or dependencies behave as expected, etc. I hear the concerns about the potential negative impacts of overly aggressive refactoring, but I think much of what is being discussed here can be accomplished without drastic changes to the existing ERDDAP code.

I put together a demonstration PR to show how Axiom has handled adding unit tests to the upstream ERDDAP while making as few changes as possible. To be clear, I am not necessarily advocating for the approach shown here to be adopted or to be the end goal. A standard Maven directory structure and surgical changes to the ERDDAP code to increase flexibility/remove assumptions as Clifford outlined above is surely the most correct and least surprising approach. Given that he's already got this working against 2.18 I think some step-wise PRs against main as discussed above would be ideal.

In the meantime, here's an PR how we've approached adding tests, which also shows an approach to separate unit tests requiring (potentially failing/slow/etc) remote resources in to a separate execution profile using jUnit categories.

#116

The overall approach is that we stage the build assets into a war-staging directory which mimics the directory structure of a deployed ERDDAP war. This allows ERDDAP to initialize successfully, find resources at the assumed paths, etc. Also required is the alteration of a stock setup.xml file with replacement of disallowed values (enforced by EDStatic) with dummy values to allow ERDDAP to successfully initialize. A fake content directory containing the doctored setup.xml is configured using -DerddapContentDirectory=./content/erddap-fake in a .mvn/maven.config file. Once the tests pass, we use the war-staging directory to build the final war archive.

In the pom there are long lists of resources to include for both images and non *.java WEB-INF files. There are (or were at the time of the creation of the initial approach) many unused files in the images directory that made it undesirable to just include the entire folder. A similar problem exists for the *.javaINACTIVE files that are already excluded in the main pom.xml. A general clean-up of these files (move unused files to a directory outside of the resource directories used in the build etc) would greatly simplify the build; even better would be the organization of these resources into standard Maven src/main/{java,webapp,resources} directories as suggested above.

If nothing else this shows the hoops that have to be jumped through/pain points to add portable tests and build methodology with the existing layout.

[srstsavage]

A couple follow up thoughts...

I forgot to provide the example text commands for the linked PR. To execute "normal" tests not requiring remote resources, our old friend

mvn test

to execute the remote test profile:

mvn test -Premote-test

There are of course many ways to separate these tests. One could also treat them as integration tests, name the test classes so they end in IT, and use the maven-failsafe-plugin to execute them during the integration-test phase. I didn't like this approach as much because the natural integration-test phase comes after packaging (war building) and introduces a lot of unnecessary delay before the remote tests are actually run. Additionally, the shown jUnit category approach allows for the creation of additional categories and test profiles that might be useful.

Also, Clifford's proposal to explicitly initialize EDStatic using a method (and I assume lazy synchronized initialization) could include an argument specifying whether or not to enforce non-default/legitimate values in setup.xml (or maybe whether to bother reading them at all?) and eliminate the need to patch them up before running tests.

[ChrisJohnNOAA]

Would it make sense to make a pull request with some of your changes after the initial changes from Clifford are integrated?

Also for the test separation, based on the latest JUnit, I believe we should be using Tags:
https://junit.org/junit5/docs/current/user-guide/#running-tests-tags
https://junit.org/junit5/docs/current/user-guide/#running-tests-build-maven-filter-tags

Is there a reason you’d recommend Categories instead of Tags?

[srstsavage]

Would it make sense to make a pull request with some of your changes after the initial changes from Clifford are integrated?

Sure, and depending on how far Clifford's changes go I'm not sure if any of the workarounds I show in my PR will be needed. But definitely happy to see where we end up and make any further suggestions at that point.

Is there a reason you’d recommend Categories instead of Tags?

Nope! I'm less experienced with jUnit 5 and tags seem like the modern way to do this type of filtering. Mostly was just demonstrating the value in separating those remote-dependent tests from the main suite that should be able to run in isolation.

[ChrisJohnNOAA]

Based on the comments here, I’m onboard with going ahead and getting pull requests from @clifford-harms to start making the proposed changes. It sounds like the changes will be smaller than I was initially expecting and will help move towards a better developer onboarding experience.

[ChrisJohnNOAA]

Getting a working test suite has been a priority for me. I have a branch where I've migrated all of the tests into test files compatible with JUnit. https://github.com/ChrisJohnNOAA/erddap/tree/test_migrations
I'm currently trying to catch any flaky tests and any paths I've missed updating to be project based before merging that in.

These tests currently rely on data downloaded from a GitHub release https://github.com/ERDDAP/erddapTest/releases/tag/rc-test. The test data is larger than I think we should include in the main ERDDAP repo and has a lot of files >10mb (relevant because it's the "limit" for file size that GitHub allows in a repo). If there's a better solution, I'd love to improve this.

There's a lot of room for improvement in my test migration branch. For example, I'd love to handle EDStatic better and think @clifford-harms approach to encapsulating EDStatic would be great to include.

There are a lot of tests in my branch with annotations (@tag[Something]) that are not run when you run mvn test. The tag indicates a reason why they aren't run. Of particular interest to this conversation is @TagLocalERDDAP which indicates tests that depend on having a local ERDDAP running.

For running a local ERDDAP there are a few options:

1. Docker
2. Jetty (example here: main...mwengren:erddap:jetty-dev-2024 thanks to @mwengren )
3. Anything else? For some of the tests that are currently tagged LocalERDDAP, we may be able to rewrite from a url request to directly call Erddap:doGet

Just a note on the Jetty approach- it is possible to run a Jetty server as part of JUnit tests, here's an example of doing that (not with erddap) https://gist.github.com/edenir-anschau/0f7dba1b72e901a340dc486a77b20fbf

Because Jetty is configurable and runnable in JUnit tests, I'm leaning towards that as the solution for testing. I'm happy to hear arguments for other approaches.

I'm happy for any contributions to the test work. I'm hoping in the next day or two I'll be able to merge my branch in. At that point there are a number of great ways to contribute to the tests (in no particular order):

1. Encapsulate EDStatic so we can better handle that in tests.
2. Develop a local server (Jetty?) solution to enable the TagLocalERDDAP tests.
3. Debug/make stable the TagFlaky tests.
4. Find solutions to issues preventing other Tag tests.
5. Anything else to help improve the tests/developer experience (finding the slow tests and either speeding them up or setting up a slow/integration test command).

[jcermauwedu]

All great stuff -- more reading to do. Will look forward to the merge and pass along PRs if they look promising. @srstsavage If there is any additional tidbits to add, that would be great too.

[ChrisJohnNOAA]

I think my test migration is more or less working now:
#142

It's massive and I'm well aware there is more work that can be done on the tests (see my previous post for some examples). I believe it is currently in a state that is stable enough to share and merge in. If anybody wants to try it out and let me know if it works for them, that'd be appreciated.

[jcermauwedu]

@ChrisJohnNOAA Still has the checksum mismatch. Otherwise, looks good to go.

70.14 [INFO] Results:
70.14 [INFO] 
70.14 [ERROR] Errors: 
70.14 [ERROR]   HashDigestTests.basicTest:21 » Runtime 
70.14 ERROR in Test.ensureEqual(Strings) line #1, col #1 '3[end]'!='9[end]':
70.14 Specifically, at line #1, col #1:
70.14 s1: 327fbb2aa6c6297d4fdd5fdf4b14e289[end]
70.14 s2: 96e9b4d974b734874a66f433b276a41a[end]
70.14     ^
70.14 
70.14 [INFO] 
70.14 [ERROR] Tests run: 57, Failures: 0, Errors: 1, Skipped: 0
70.14 [INFO] 
70.14 [INFO] ------------------------------------------------------------------------
70.14 [INFO] BUILD FAILURE
70.14 [INFO] ------------------------------------------------------------------------
70.14 [INFO] Total time:  01:07 min
70.14 [INFO] Finished at: 2024-04-13T12:16:53Z
70.14 [INFO] ------------------------------------------------------------------------

[ChrisJohnNOAA]

@jcermauwedu #142 is not merged into main yet. The test is fixed in that branch. There's some differences between windows and linux machines that I'm trying to fix with Shane's help before I merge.