Testing updates #494

peverwhee · 2023-09-01T16:46:49Z

Summary

This PR contains updates to the testing infrastructure, bringing in CI.

Description

Add github workflow to automatically run tests on PR opening
Update doctests to run w/o main
Temporarily comment out unit conversion test from workflow (until implemented)

User interface changes?: No

gold2718

I have a couple of questions / observations but nothing that should hold up this PR.

gold2718 · 2023-09-02T08:13:47Z

.github/workflows/capgen_unit_tests.yaml

+
+jobs:
+  unit_tests:
+    if: github.event_name == 'pull_request' || github.event_name == 'workflow_dispatch' || github.repository == 'NCAR/ccpp-framework'


I am admittedly weak on the GitHub CI stuff but would this activate on something that happens that is not a PR because of the logical OR statements?
This logic is repeated in other files so I wil just ask this once since it is probably something I just don't understand (yet).

The first two statements make sense. Either trigger on a PR or when someone runs the workflow manually from the "actions" tab (workflow dispatch). The last statement doesn't make sense to me, because it basically says always run when the repo is the NCAR repo. Was the intent to trigger on one of the two preceding statements only if it's the NCAR repo? Then I would have expected something like (pseudo-syntax, because I actually don't know if github actions accept complex logical expressions)

if (pull_request || workflow_dispatch) && repo == ncar

I am still fairly new to GitHub actions so this could possibly be simplified but he intent was to:

Run the action on any change to the NCAR repo

Run the action on pull requests (This may need to be updated as the workflows take longer)

Allow manually running actions on forked repositories outside of pull requests.
If there's better ways to support this in the workflow files, just let me know!

@mwaxmonsky I don't think you need to worry about adding logic, just set "on" to pull_request and workflow_dispatch. This will run the test when any change is introduced via PR and allow for manual running.
There are some CI examples in the NCAR:ccpp-scm repository. Some tests are more exhaustive and only triggered for PR, other small tests are run with every push.

Hi All,

I was the one who told @mwaxmonsky and @peverwhee to add this logic. The reason is because we want these tests to trigger whenever a push occurs, but only to the NCAR repo (not forks). The logic above basically says "Run these tests if it is a pull request event, the workflow is triggered manually, OR it is a push event, but the push is occurring in the NCAR repo".

This is because folks might sometimes forget to merge their branch to the head of the NCAR branch, and thus when they "merge" the PR in it might result in a test failure that we wouldn't catch unless the tests are run one last time after the PR is merged in (which in Github world counts as a "push"). The other situation this might matter is if someone directly pushes to the NCAR repo. I realize that is generally a no-no, but I've seen it happen at least once in every repo I have worked on (usually by accident).

Of course an alternative would be to have these tests run for every push event, regardless of where, or to have the push event only occur on certain branches (e.g. feature/capgen). However, I personally don't like having tests run all the time when I am doing development on my own fork (especially if I know I am breaking things), and making it branch-specific might potentially cause issues down the road when we unify the framework. So this was the solution we landed on.

Anyways, I'm happy to change this logic if folks would like a different set of triggering events, but just wanted to try and explain it here first. Thanks!

I don't think it's needed. Whenever you "push" a commit to a PR, which includes merging the latest develop in (on the command line, then push, or via the button on the PR page), the tests are triggered with the pull_request condition.

Simply enforce in the branch protection that PRs need to be up to date with develop and you're done.

That's a good point about the branch protection rules! If we implement that along with rules preventing any direct pushes to the NCAR repo (which may already exist?) then I agree that this can all go away.

Happy to go with you through the setup this afternoon or later.

@climbfuji Jesse checked and doesn't have permission to do so, so if you could instead walk me through it, I would appreciate that!

gold2718 · 2023-09-02T08:17:16Z

scripts/ccpp_state_machine.py

+_INIT_ST = r"(?:init(?:ial(?:ize)?)?)"
+_FINAL_ST = r"(?:final(?:ize)?)"
+_RUN_ST = r"(?:run)"
+_TS_INIT_ST = r"(?:timestep_init(?:ial(?:ize)?)?)"
+_TS_FINAL_ST = r"(?:timestep_final(?:ize)?)"


This is a dup of #493, is it needed in both places?

gold2718 · 2023-09-02T08:26:12Z

scripts/state_machine.py

@@ -30,7 +30,7 @@ class StateMachine:
    >>> StateMachine([('ab','a','b','a')]).final_state('ab')
    'b'
    >>> StateMachine([('ab','a','b','a')]).transition_regex('ab')
-    re.compile('a$')
+    re.compile('a$', re.IGNORECASE)


Changes in this file also seem to mirror #493 but I don't see an obvious relationship between the branches since dc6458e.
Might make merging later more fun?

Hopefully those changes were cherry-picked or the two PRs started from the same branch, in which case there won't be any merge conflicts. But in any case #493 should be merged first, then the update from feature/capgen pulled into this branch, and these diffs should go away magically.

@climbfuji is correct. I had included them in the cleanup PR but then needed the updates in this PR to get the tests to pass. After I merge #493 I will confirm this diffs go away.

gold2718 · 2023-09-02T08:28:28Z

scripts/var_props.py

+        # Initial setup
+        >>> from parse_tools import init_log, set_log_to_null
+        >>> _DOCTEST_LOGGING = init_log('var_props')
+        >>> set_log_to_null(_DOCTEST_LOGGING)
+        >>> _DOCTEST_RUNENV = CCPPFrameworkEnv(_DOCTEST_LOGGING, \
+                                               ndict={'host_files':'', \
+                                                      'scheme_files':'', \
+                                                      'suites':''}, \
+                                               kind_types=["kind_phys=REAL64", \
+                                                           "kind_dyn=REAL32", \
+                                                           "kind_host=REAL64"])
+        >>> _DOCTEST_CONTEXT1 = ParseContext(linenum=3, filename='foo.F90')
+        >>> _DOCTEST_CONTEXT2 = ParseContext(linenum=5, filename='bar.F90')
+        >>> _DOCTEST_VCOMPAT = VarCompatObj("var_stdname", "real", "kind_phys", \
+                                            "m", [], "var1_lname", "var_stdname", \
+                                            "real", "kind_phys", "m", [], \
+                                            "var2_lname", _DOCTEST_RUNENV, \
+                                            v1_context=_DOCTEST_CONTEXT1, \
+                                            v2_context=_DOCTEST_CONTEXT2)
+


Stay tuned viewer, you won't believe what's up next on "Doctests gone wild"!

dustinswales

@peverwhee Looks good. A couple small comments:

Adding a ccpp_prebuild test to the CI (I can help with this)
There are identical changes as in Capgen cleanup #493. They look small, but may cause cmall issues down the road.

dustinswales · 2023-09-06T14:52:15Z

.github/workflows/capgen_unit_tests.yaml

+
+jobs:
+  unit_tests:
+    if: github.event_name == 'pull_request' || github.event_name == 'workflow_dispatch' || github.repository == 'NCAR/ccpp-framework'


@mwaxmonsky I don't think you need to worry about adding logic, just set "on" to pull_request and workflow_dispatch. This will run the test when any change is introduced via PR and allow for manual running.
There are some CI examples in the NCAR:ccpp-scm repository. Some tests are more exhaustive and only triggered for PR, other small tests are run with every push.

dustinswales · 2023-09-06T15:02:41Z

.github/workflows/capgen_unit_tests.yaml

@@ -0,0 +1,22 @@
+name: Capgen Unit Tests


@gold2718 @climbfuji @mwaxmonsky @peverwhee
It would be nice if we had a CI test in the feature_capgen branch that ran the ccpp_prebuild.

We do run ccpp_prebuild as part of the ccpp-scm and ccpp-physics CI (e.g. https://github.com/ufs-community/ccpp-physics/blob/ufs/dev/.github/workflows/ci_scm_ccpp_prebuild.yml). One could easily modify this to run ccpp_prebuild using the feature_capgen fork, which would let us know if capgen development breaks the existing prebuild.
Thoughts? I can add this test to this PR if there is interest. Personally, I'd like to see this.

I personally think this is a great idea! Our original thought was that if we could "unify" the testing system first then it would make unifying the actual framework easier. Of course @peverwhee and @mwaxmonsky are the ones actually responsible for all of this so I'm happy to let them make the final decision (at least for the NCAR side).

Note that we already have ccpp_prebuild.py tests in main:

In stub to build a CCPP stub that exercises the framework. See README.md in this directory. Annoyingly, this seems to be broken right now, at least on my macOS (I'll look into fixing this later today).

In subdirectory tests (not the s at the end, the test directory is for capgen). These can be run via

cd tests PYTHONPATH="$PWD/../scripts:$PWD/../scripts/parse_tools:$PYTHONPATH" python3 test_mkstatic.py PYTHONPATH="$PWD/../scripts:$PWD/../scripts/parse_tools:$PYTHONPATH" python3 test_metadata_parser.py

I have a local workflow file that can do this similar to ccpp-physics but after discussing with @peverwhee, we are going to add that in a separate PR because it fails in the current state but successfully runs when merged with the main branch.

created issue #500 for prebuild CI integration

.github/workflows/capgen_unit_tests.yaml

.github/workflows/python.yaml

climbfuji · 2023-09-06T21:52:20Z

scripts/state_machine.py

@@ -30,7 +30,7 @@ class StateMachine:
    >>> StateMachine([('ab','a','b','a')]).final_state('ab')
    'b'
    >>> StateMachine([('ab','a','b','a')]).transition_regex('ab')
-    re.compile('a$')
+    re.compile('a$', re.IGNORECASE)


Hopefully those changes were cherry-picked or the two PRs started from the same branch, in which case there won't be any merge conflicts. But in any case #493 should be merged first, then the update from feature/capgen pulled into this branch, and these diffs should go away magically.

mwaxmonsky

Aside from the proposed workflow CI changes mentioned above I need to push up, looks good to me!

mwaxmonsky

...Forgot to click the Approved button. 😄

climbfuji · 2023-10-02T21:06:45Z

.github/workflows/python.yaml

+        pytest -v
+
+  doctest:
+    if: github.event_name == 'pull_request' || github.event_name == 'workflow_dispatch' || github.repository == 'NCAR/ccpp-framework'


Do we still need the complicated logic here?

good catch! removed.

climbfuji

Thanks for addressing my comments!

Courtney Peverley and others added 5 commits August 21, 2023 13:35

initial stab at testing-only PR

43031fe

add latest changes

1683be3

fix grammar

b085139

remove tests that should be in constituents branch

a308887

remove testing files

a1aaf9e

peverwhee requested a review from climbfuji as a code owner September 1, 2023 16:46

peverwhee self-assigned this Sep 1, 2023

peverwhee requested a review from gold2718 as a code owner September 1, 2023 16:46

peverwhee requested review from dustinswales and mwaxmonsky September 1, 2023 16:47

Courtney Peverley added 3 commits September 1, 2023 11:09

update regexes in state_machine

4247019

fix doctest

104e18f

add state_machine updates

bbdf0d0

gold2718 approved these changes Sep 2, 2023

View reviewed changes

dustinswales reviewed Sep 6, 2023

View reviewed changes

climbfuji reviewed Sep 6, 2023

View reviewed changes

dustinswales mentioned this pull request Sep 14, 2023

Run Travis on all forks? #401

Closed

mwaxmonsky reviewed Sep 18, 2023

View reviewed changes

mwaxmonsky approved these changes Sep 18, 2023

View reviewed changes

peverwhee mentioned this pull request Sep 25, 2023

Integrate prebuild tests into capgen CI #500

Closed

merge up to head of feature/capgen

c2ffb05

peverwhee requested review from grantfirl and mkavulich as code owners September 25, 2023 19:08

simplify workflow

e75e14a

peverwhee requested a review from climbfuji September 25, 2023 20:01

climbfuji reviewed Oct 2, 2023

View reviewed changes

remove unnecessary logic from workflow

d58face

climbfuji approved these changes Oct 2, 2023

View reviewed changes

Courtney Peverley added 2 commits October 12, 2023 16:00

merge up

b006850

update metadata parser test with cleanup

07828e6

fix merge

1bacf44

peverwhee merged commit eac8bb8 into NCAR:feature/capgen Oct 12, 2023
11 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Testing updates #494

Testing updates #494

peverwhee commented Sep 1, 2023

gold2718 left a comment

gold2718 Sep 2, 2023

climbfuji Sep 5, 2023

mwaxmonsky Sep 5, 2023

dustinswales Sep 6, 2023

nusbaume Sep 6, 2023

climbfuji Sep 6, 2023

climbfuji Sep 6, 2023

nusbaume Sep 6, 2023

climbfuji Sep 6, 2023

peverwhee Sep 6, 2023

gold2718 Sep 2, 2023

gold2718 Sep 2, 2023

climbfuji Sep 6, 2023

peverwhee Sep 25, 2023

gold2718 Sep 2, 2023

dustinswales left a comment

dustinswales Sep 6, 2023

dustinswales Sep 6, 2023

nusbaume Sep 6, 2023

climbfuji Sep 6, 2023

mwaxmonsky Sep 18, 2023

peverwhee Sep 25, 2023

climbfuji Sep 6, 2023

mwaxmonsky left a comment

mwaxmonsky left a comment

climbfuji Oct 2, 2023

peverwhee Oct 2, 2023

climbfuji left a comment

Testing updates #494

Testing updates #494

Conversation

peverwhee commented Sep 1, 2023

Summary

Description

gold2718 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dustinswales left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mwaxmonsky left a comment

Choose a reason for hiding this comment

mwaxmonsky left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

climbfuji left a comment

Choose a reason for hiding this comment