-
Notifications
You must be signed in to change notification settings - Fork 25
Add unit-level regression test for normative rule extraction scripts #74
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
base: main
Are you sure you want to change the base?
Conversation
…ted results Signed-off-by: James Ball <[email protected]>
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.
Generally looks good to me!
Is there an existing github workflow that this could be added to?
|
||
DOCKER_BIN ?= docker | ||
SKIP_DOCKER ?= $(shell if command -v ${DOCKER_BIN} >/dev/null 2>&1 ; then echo false; else echo true; fi) | ||
DOCKER_IMG := riscvintl/riscv-docs-base-container-image:latest |
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 don't know if I would bother with the complexity of docker for this. The only reason it's needed in riscv-isa-manual
is because setting up Asciidoctor and all the plugins is a right pain, but I think it's probably an acceptable pain for people wanting to run these tests (and CI). Especially because the only requirement is Asciidoctor I think?
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.
Yeah, I don't need any of the Asciidoctor plugins. Obviously I'm leveraging the docker image and Makefile from the ISA manual. If I don't do this, then as you point out, people will need to have Asciidoctor installed to run the tests. I don't have Asciidoctor installed on my Windows machine (I run this on WSL2 with docker) so that would open a whole new can of worms if I now need to have Asciidoctor installed. So, in summary, I'd like to keep this they way it is since people are already familiar with using the docker image in the ISA manual. Do you agree?
cd [email protected] && | ||
endif | ||
|
||
WORKDIR_SETUP = \ |
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.
You can probably skip this workdir stuff too.
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.
Just being "efficient" by leveraging the ISA manual Makefile. Seems to work well so why fix it if it ain't broke?
Signed-off-by: James Ball <[email protected]>
…ignore Signed-off-by: James Ball <[email protected]>
Getting pre-commit failure for expected JSON files that don't have extra line at the end (not a requirement for JSON files). Submitted #76 to get this fixed. |
I was wondering that too. Doesn't look like is anything like that in this repo. UDB has this kind of thing so I've seen it run on the GitHub servers during a PR. That would be ideal to setup for this docs-resources repo too. That will take someone with GitHub actions expertise (which I don't have). I could learn it but given all on my plate I don't think I can justify that. @rpsene, is this something you could help with? I think you are already up-to-speed on GitHub actions and PR checks. |
Fixes #73
Add test.adoc and Makefile to ensure tags and norm rules create expected results.