Skip to content
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

Add sequences into report #736

Merged
merged 19 commits into from
Nov 1, 2023
Merged

Conversation

noursaidi
Copy link
Collaborator

@noursaidi noursaidi commented Oct 9, 2023

Adds sequences into report. Link to example report below.

CI testing additions just need to be fixed (which is a check that the sequence.md files in validator/sequences are up to date , otherwise there is a risk of comparing to an out of date sequence.md file)

Sample report (click here) - the sequences.md don't actually match the results, just for illustration

@noursaidi noursaidi requested a review from grafnu October 9, 2023 17:20
bin/sequencer Outdated
@@ -158,6 +158,6 @@ fi
fgrep RESULT $site_model/out/devices/$device_id/RESULT.log | \
fgrep ' schemas ' | sort -k 3 | tee $SCHEMA_OUT

(source venv/bin/activate && bin/sequencer_report $site_model $device_id)
(source venv/bin/activate && python3 bin/sequencer_report.py $site_model $device_id)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rather than make them explicitly python, I think a better solution would be to do something like

PATH=venv/bin:$PATH bin/sequencer_report $site_model $device_id -- so it will pick up the venv as needed.

@@ -135,4 +135,10 @@ diff -u out/generated.md docs/specs/sequences/generated.md || (
echo Checking for duplication in out/sequencer.out and etc/sequencer_planning.txt
bin/test_sequencer_plan

echo Checking that the sequences in validator/sequences are correct
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't this be checking the sequences in the generated site_model (out/) are correct (verified against validator/sequences)?

I think part of the problem here is the overall generation/updating of these things is a bit... wonky? I'm not happy with whatever hack I have in place so some of this stuff is confusing me (even before you touched it at all!)

@@ -135,4 +135,10 @@ diff -u out/generated.md docs/specs/sequences/generated.md || (
echo Checking for duplication in out/sequencer.out and etc/sequencer_planning.txt
bin/test_sequencer_plan

echo Checking that the sequences in validator/sequences are correct
ls -1 sites/udmi_site_model/out/devices/AHU-1/tests/ \
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My concern here is that this would ultimately end up being too unstable, since it would be checking all the alpha/etc... tests. Also, we should just make sure this is reconciled with the bit in test_sequencer that checks the generated.md file, since it's functionally supposed to be the same thing.

@noursaidi
Copy link
Collaborator Author

@grafnu PTAL again

@noursaidi noursaidi requested a review from grafnu November 1, 2023 07:23
Copy link
Collaborator

@grafnu grafnu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh boy -- this is going to have some fun merge conflicts when I pull it into my current PR (just changing the testing structure)... but I'll sort all that out as needed.

@noursaidi noursaidi merged commit 4cbbbbb into faucetsdn:master Nov 1, 2023
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants