Our Testing needs attention

[Brennan1994]

There's an issue with our testing framework, and we're getting bit by it too often for my tastes.

I'm working through some failing tests in InventoryShould.cs today that are results of edits made over the last couple months that I suspect weren't caught because when we ran the tests on the pull requests on github, the green check came back. Trouble is InventoryShould.cs is full of disk tests that don't run on GitHub, so we missed it.

This happens the same with integration tests. Those do actually run on Github weekly, but the results are hidden from the main page, so we never pay attention to them.

I'm thinking moving forward we may want to do more mock data things like I did with hydraulics. I see huge value in unit tests that serve as a gatekeeper to merging a pull request, and drastically less value in integration tests which run once a week that nobody looks at, and even less in disk tests that we only run on our machines for development.

I have ideas to make this better I'll record here. I just wanted to start this discussion while it was fresh in my head. Also open to other opinions and ideas.

[rnugent3]

@Brennan1994 it has been on my mind, too. Thanks for starting this discussion. Setting up more mock objects sounds like a great idea for our unit and integration tests.

Is there a procedure you'd recommend for some manual testing to make sure we're importing and reading files correctly?

[Brennan1994]

https://stackoverflow.com/questions/1408175/execute-unit-tests-serially-rather-than-in-parallel

describes how to make Xunit run tests sequentially rather than parallel. That would remove our issues with test failing due to file access things for the files we have in the repo. Some combination of this and more thoughtfully separating test classes is probably what we need.

If we could get all the file loading things to be a single, separate test class, we could load up super simple datasets, just complex enough to prove they load, and then migrate all other classes to using mock data.

Any test classes that are meant to benchmark/test real world datasets should only be separate from the ones that just confirm behavior. And the data for those classes should be hosted somewhere (like devnet drive), where we can manually download and place in a specified location on our machines to run the tests.

There's also a way to add a little widget to our front page that would put the status of our integration tests front and center. Might drive more attention toward them. I've seen it on other repos. I'll need to investigate how to make that happen for us, but I think that might be useful.

That's a lot of babble. Trying to boil down a level...

• Unit tests should confirm expected behavior and run quickly. (files load correctly). The data we need for them can be pretty tiny. We'll need to store some binaries in git, but we can make them miniscule.
• Disk tests can be used to run real world datasets, which should be stored separately from the repo and not run on actions.
• Integration tests I kind of want to just rename weekly tests. That's the defining characteristic. It's unit tests that test too large a unit to run quickly as a unit test, so we only run these weekly.
• ALL classes which use a file on disk in their test need to be tagged with a Collection attribute to ensure they run serial, rather than parallel. These should basically be limited to testing that we can load the elements from disk we need. That's steady & unsteady hdfs, WSE grids, Structure inventories and impact areas, and the ASCII imports.

[Brennan1994]

I did some investigation. I have findings and recommendations.

• Github actions doesn't handle computes well. Tests that take more than 5 or so seconds on our local machines can REALLY hang actions. I don't understand exactly why, but I've done a bunch of testing of it and can prove it to be so.
• Integration tests aren't very useful unless we use them as a barrier to merging, and we can't use them that way because github is too slow.

My recommendations (implemented on branch refactor/testOrganization):

• I've removed the attribute catagory "Integration" from our test classes. The new attribute scheme is simply RunsOn = Local or RunsOn = Remote. quick tests ( < 5sec locally) can be remote tests. There is huge value in making things remote, so moving forward we need to use this tag a liberally as possible.
• Classes that were previously labeled integration are now mostly labeled Local. As part of review, we need to be running all the local tests on our machine to ensure they pass. This is less automated and not super ideal, but ultimately I don't think there was any value in automating a weekly test, and if we can build it into our SOP to run them on pull request, I think they'll be immensely beneficial again.

These changes are reflected on PR769 #769

[rnugent3]

Sounds good. Let's do it. @Brennan1994 would you mind updating our contributing guide to reflect that?

[Brennan1994]

Done.