Action to check engine against published rules#1700
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| - name: Checkout cdisc-open-rules | ||
| uses: actions/checkout@v6 | ||
| with: | ||
| repository: cdisc-org/cdisc-open-rules | ||
| ref: ${{ inputs.rules_ref || 'main' }} | ||
| path: open-rules | ||
| # If cdisc-open-rules is private, add a PAT secret: | ||
| # token: ${{ secrets.CDISC_OPEN_RULES_TOKEN }} | ||
|
|
||
| # ----------------------------------------------------------------------- | ||
| # 3. Set up Python | ||
| # ----------------------------------------------------------------------- | ||
| - name: Set up Python 3.12 |
gerrycampion
left a comment
gerrycampion
left a comment
There was a problem hiding this comment.
This is looking really nice, but I have some comments
| uses: actions/checkout@v6 | ||
| with: | ||
| repository: cdisc-org/cdisc-rules-engine | ||
| ref: ${{ inputs.engine_ref || github.sha }} |
There was a problem hiding this comment.
remove ref for engine checkout. It should use whatever the default is.
gerrycampion
left a comment
gerrycampion
left a comment
There was a problem hiding this comment.
The summary and detailed outputs look awesome. I have one comment, sorry I didn't pickup on this earlier. Hopefully it is an easy conversion for ai.
| # Write the JSON-line parser once; called once per test case in the loop below | ||
| cat > /tmp/parse_case.py << 'PYEOF' | ||
| import sys, json, shlex | ||
| d = json.load(sys.stdin) | ||
| for k, v in [ | ||
| ('CASE_RULE', d['rule']), | ||
| ('CASE_TYPE', d['type']), | ||
| ('CASE_NUM', str(d['num'])), | ||
| ('EXEC_OK', '\u2705' if d['exec'] else '\u274c'), | ||
| ('EXPECTED', str(d.get('expected', ''))), | ||
| ('GOT', str(d.get('got', ''))), | ||
| ('MATCH', '\u2705' if d.get('match') else '\u274c'), | ||
| ('DIFF_FILE', str(d.get('diff', ''))), | ||
| ('STDERR_FILE', str(d.get('stderr', ''))), | ||
| ]: | ||
| print(k + '=' + shlex.quote(v)) | ||
| PYEOF |
There was a problem hiding this comment.
This should just go in a python file in /scripts. In fact, I think this entire Run validation for all Published rules job after the first call to run_validation.sh should go in a python file for easier maintenance. I would copy this utility to simplify markdown table creation: https://github.com/cdisc-org/CORE_Test_Suite/blob/main/scripts/markdown_utils.py
alexfurmenkov
changed the title
| repository: cdisc-org/cdisc-open-rules | ||
| ref: ${{ inputs.rules_ref || 'rules_2' }} | ||
| path: open-rules | ||
| # If cdisc-open-rules is private, add a PAT secret: |
There was a problem hiding this comment.
I think instead of leaving a comment like this we should either use it or totally remove to basedon the requirement.
| uses: actions/checkout@v6 | ||
| with: | ||
| repository: cdisc-org/cdisc-open-rules | ||
| ref: ${{ inputs.rules_ref || 'rules_2' }} |
There was a problem hiding this comment.
here the system will always fallback to using branch rules_2 when it is auto triggered because of push to main. Input.rules_ref is set on manual override. Please let me know if I am misunderstanding the logic.
|
|
||
| if not os.path.isdir(published_dir): | ||
| print(f"WARNING: Published/ not found under {rules_root}", file=sys.stderr) | ||
| return False |
There was a problem hiding this comment.
Do you think it would be better to fail the test when published rule folder is missing, instead of printing. a warning and silently passing the test? Silently passing might create a false positive here.
|
Thanks for the updates. The PR seems ready to me for merge now, this s this PR depends on changes in PR we will wait for it to be merged before approving this one. |
4 participants
workflow to run validation for all published rules, comparing with benchmark validation results
cdisc-org/cdisc-open-rules#22 with changes to allow validation from this repo