Test actions

[mwaxmonsky]

Adding Unit Test Action for CI

Adds new Dockerfile for test container to run unit tests in.

User interface changes?: No

Fixes: None

Testing:
test removed: var_action_suite (temporarily disabled)
unit tests: all in /test
system tests: None
manual testing: None

[nusbaume]

Looks like a great start! I did have a couple questions and a few (hopefully minor) requests, though.

[nusbaume]

Question for @peverwhee and @mwaxmonsky. Right now this workflow will run even when people push code to their own branches on their own forks. This of course provides the benefit of testing early and often, but I know personally I sometimes get annoyed when I know I am breaking a test during development on my local fork but keep getting complaints from Github about it.

An alternative is we can have this workflow only run on commits to already open pull requests (to anywhere), and to commits that are made solely to the main NCAR/ccpp-framework repo. To do this you would need to add the following line under the unit_tests line:

if: github.event_name == 'pull_request' || github.repository == 'NCAR/ccpp-framework'

Since I suspect you two will be doing most of the development in this repo I'll leave the decision up to you.
I just wanted to bring this up as something to think about (as I have definitely run into it in other repos).

[mwaxmonsky]

This is a great point and is actually something I've run into with previous organizations with similar resolutions. I'm all onboard with more judicious use of the actions especially if they can always be manually triggered if needed.

@peverwhee Any thoughts or objections?

[peverwhee]

@nusbaume @mwaxmonsky I agree that only triggering the tests for pull requests to the main repo is ideal!

[mwaxmonsky]

The branch has been updated but I'm having a small problem with getting jobs to run. I tried a few ways of implementing it and the current way should work but I must be missing a small detail.

Currently the manual dispatch will trigger but the job will still say skipped on my repo. Going to continue to investigate.

[nusbaume]

This reminds me that it might be good to get a pylint Github Action job up and running as well, to make sure the CCPP python scripts are fully analyzed each time. I don't think we need it for this PR, but it could be good to make an issue for it. Thoughts?

[mwaxmonsky]

I'm always on board with more testing! Especially if we're going to be adding more python as we go, would it also be a good idea to add a formatter as well?

[nusbaume]

I have no problem with implementing a formatter, but it would probably be good to double-check with @peverwhee first as she is the other core developer for this framework.

[peverwhee]

I think for sure pylint-ing is a good addition. Maybe no formatter (for now) as that might be too naggy...

[nusbaume]

I also forgot to mention that we might want to move the Dockerfile to be under the test directory, because it is currently only being used for testing the capgen (NCAR) branch, and we don't want to mistakenly imply that it can be used for the pre-build (NOAA) branch as well. Of course once we unify both branches then we can hopefully also provide a single unified container.

[nusbaume]

Everything generally looks good now. However, after reflecting on the last CCPP-framework meeting discussion I think for the moment it might be safer to just try and use the ubuntu-latest virtual machine instead of bringing in our own container.

[nusbaume]

Super-close! Just one last question.

[nusbaume]

Looks great now, thanks @mwaxmonsky!

[nusbaume]

@mwaxmonsky My guess is that something is off with the if: github.event_name... line. It might be worth printing the github.event_name to stdout before the if-statement to make sure its value is what we think it should be. I believe you can do that by simply adding echo ${{ github.event_name }} to the workflow file just before the if-statement, although if that doesn't work please let me know.

[mwaxmonsky]

@nusbaume Ah ha, I think I got it working. I'm not sure why but it looks like the workflow dispatch option needs to be first in the actions list. Not sure if this is by design or a bug but I can manually trigger actions on my branch now.

[nusbaume]

@mwaxmonsky Great, glad you figured it out! Yeah I have no idea why it needs to come first, but good to know for any future workflows!

[peverwhee]

@mwaxmonsky I know I said "if it's easy, go ahead and throw in a pylint test" but now I'm thinking there's more work to be done on linting all of the python files (or at least those that were updated). As written, this runs pylint on the test files, which is probably not quite sufficient.

My vote (sorry to make you undo things!) is to remove this and make an issue.

[mwaxmonsky]

Yeah, I was hoping that since there was a .pylintrc file already, this would be a fairly quick add but it looks like that file hasn't been touched in a while and there's a few things that need to be examined so I'll remove this from here and make a separate PR. No worries!