-
Notifications
You must be signed in to change notification settings - Fork 64
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Testing updates #494
Testing updates #494
Changes from 8 commits
43031fe
1683be3
b085139
a308887
a1aaf9e
4247019
104e18f
bbdf0d0
c2ffb05
e75e14a
d58face
b006850
07828e6
1bacf44
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,22 @@ | ||
name: Capgen Unit Tests | ||
|
||
on: | ||
climbfuji marked this conversation as resolved.
Show resolved
Hide resolved
|
||
workflow_dispatch: | ||
pull_request: | ||
types: [opened, synchronize, reopened] | ||
push: | ||
branches: | ||
#Trigger workflow on push to any branch or branch hierarchy: | ||
- '**' | ||
|
||
jobs: | ||
unit_tests: | ||
if: github.event_name == 'pull_request' || github.event_name == 'workflow_dispatch' || github.repository == 'NCAR/ccpp-framework' | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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)
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am still fairly new to GitHub actions so this could possibly be simplified but he intent was to:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @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 was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Simply enforce in the branch protection that PRs need to be up to date with develop and you're done. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Happy to go with you through the setup this afternoon or later. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @climbfuji Jesse checked and doesn't have permission to do so, so if you could instead walk me through it, I would appreciate that! |
||
runs-on: ubuntu-latest | ||
steps: | ||
- uses: actions/checkout@v3 | ||
- name: update repos and install dependencies | ||
run: sudo apt-get update && sudo apt-get install -y build-essential gfortran cmake python3 git | ||
- name: Run unit tests | ||
run: cd test && ./run_fortran_tests.sh | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,28 +1,55 @@ | ||
name: Python package | ||
|
||
on: [push] | ||
on: | ||
climbfuji marked this conversation as resolved.
Show resolved
Hide resolved
|
||
workflow_dispatch: | ||
pull_request: | ||
types: [opened, synchronize, reopened] | ||
push: | ||
branches: | ||
#Trigger workflow on push to any branch or branch hierarchy: | ||
- '**' | ||
|
||
jobs: | ||
build: | ||
|
||
if: github.event_name == 'pull_request' || github.event_name == 'workflow_dispatch' || github.repository == 'NCAR/ccpp-framework' | ||
runs-on: ubuntu-latest | ||
strategy: | ||
matrix: | ||
python-version: [3.7] | ||
python-version: ['3.7', '3.8', '3.9', '3.10', '3.11'] | ||
|
||
steps: | ||
- uses: actions/checkout@v2 | ||
- uses: actions/checkout@v3 | ||
- name: Set up Python ${{ matrix.python-version }} | ||
uses: actions/setup-python@v2 | ||
uses: actions/setup-python@v4 | ||
with: | ||
python-version: ${{ matrix.python-version }} | ||
- name: Install dependencies | ||
run: | | ||
python -m pip install --upgrade pip | ||
pip install flake8 pytest | ||
if [ -f requirements.txt ]; then pip install -r requirements.txt; fi | ||
pip install pytest | ||
- name: Test with pytest | ||
if: github.repository == 'NCAR/ccpp-framework' # Only run on main repo | ||
run: | | ||
export PYTHONPATH=$(pwd)/scripts:$(pwd)/scripts/parse_tools | ||
pytest | ||
pytest -v | ||
|
||
doctest: | ||
if: github.event_name == 'pull_request' || github.event_name == 'workflow_dispatch' || github.repository == 'NCAR/ccpp-framework' | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we still need the complicated logic here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. good catch! removed. |
||
runs-on: ubuntu-latest | ||
strategy: | ||
matrix: | ||
python-version: ['3.7', '3.8', '3.9', '3.10', '3.11'] | ||
|
||
steps: | ||
- uses: actions/checkout@v3 | ||
- name: Set up Python ${{ matrix.python-version }} | ||
uses: actions/setup-python@v4 | ||
with: | ||
python-version: ${{ matrix.python-version }} | ||
- name: Install dependencies | ||
run: | | ||
python -m pip install --upgrade pip | ||
pip install pytest | ||
- name: Doctest | ||
run: | | ||
export PYTHONPATH=$(pwd)/scripts:$(pwd)/scripts/parse_tools | ||
pytest -v scripts/ --doctest-modules |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,2 @@ | ||
[pytest] | ||
addopts = -ra -q --ignore=tests/test_capgen.py | ||
testpaths = | ||
tests | ||
addopts = -ra --ignore=scripts/metadata2html.py --ignore-glob=test/**/test_reports.py |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,11 +3,11 @@ | |
# CCPP framework imports | ||
from state_machine import StateMachine | ||
|
||
_INIT_ST = r"(?:(?i)init(?:ial(?:ize)?)?)" | ||
_FINAL_ST = r"(?:(?i)final(?:ize)?)" | ||
_RUN_ST = r"(?:(?i)run)" | ||
_TS_INIT_ST = r"(?:(?i)timestep_init(?:ial(?:ize)?)?)" | ||
_TS_FINAL_ST = r"(?:(?i)timestep_final(?:ize)?)" | ||
_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)?)" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a dup of #493, is it needed in both places? |
||
|
||
# Allowed CCPP transitions | ||
# pylint: disable=bad-whitespace | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @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. |
||
>>> StateMachine([('ab','a','b','a')]).function_match('foo_a', transition='ab') | ||
('foo', 'a', 'ab') | ||
>>> StateMachine([('ab','a','b',r'ax?')]).function_match('foo_a', transition='ab') | ||
|
@@ -162,8 +162,9 @@ def __setitem__(self, key, value): | |
if len(value) != 3: | ||
raise ValueError("Invalid transition ({}), should be of the form (inital_state, final_state, regex).".format(value)) | ||
# end if | ||
regex = re.compile(value[2] + r"$") | ||
function = re.compile(FORTRAN_ID + r"_(" + value[2] + r")$") | ||
regex = re.compile(value[2] + r"$", re.IGNORECASE) | ||
function = re.compile(FORTRAN_ID + r"_(" + value[2] + r")$", | ||
re.IGNORECASE) | ||
self.__stt__[key] = (value[0], value[1], regex, function) | ||
|
||
def __delitem__(self, key): | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that we already have
ccpp_prebuild.py
tests inmain
:stub
to build aCCPP
stub that exercises the framework. SeeREADME.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).tests
(not thes
at the end, thetest
directory is forcapgen
). These can be run viaThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
created issue #500 for prebuild CI integration