enable additional platforms in CI #1121
Conversation
aap-sc
force-pushed
the
aap-sc/ci_extension
branch
from
6dab58a
to
9121710
Compare
in addition, extend vebosity of log files
aap-sc
force-pushed
the
aap-sc/ci_extension
branch
from
9121710
to
006680f
Compare
| @@ -115,7 +115,8 @@ jobs: | |||
| --gcc /opt/riscv/toolchain/bin/riscv-none-elf-gcc \ | |||
| --gdb /opt/riscv/toolchain/bin/riscv-none-elf-gdb \ | |||
| --sim_cmd /opt/riscv/spike/bin/spike \ | |||
| --server_cmd $GITHUB_WORKSPACE/src/openocd | |||
| --remotelogfile-enable \ | |||
| --server_cmd "$GITHUB_WORKSPACE/src/openocd -d3" | |||
There was a problem hiding this comment.
I would prefer not to add -d3 here. The reason is: d3 could change OpenOCD behavior (there are many cases of if (debug_level < 3) return; ).
There was a problem hiding this comment.
- I'm strongly against not testing -d3 . This is the mode when we use when run this testsuite internally. And this is the mode we use when we ask users when there are problems.
- The fact that there is these kinds of if statement in the codebase is a major problem, but I think it is a separate one. If it comes to that - such if statements should be eradicated. At least from riscv codebase for now.
There was a problem hiding this comment.
This is the mode when we use when run this testsuite internally.
Exactly. After this change there will be no testing of OpenOCD without -d3.
Anyway, I don't have a strong opinion on this so I'll approve the change regardless.
There was a problem hiding this comment.
@JanMatCodasip , @MarekVCodasip do you have an opinion on the matter? The concern raised by @en-sc is valid.
I'm inclined to extend testing even more by enabling both variants: "-d3" is enabled and "-d3" is omitted. Tests may be run a little bit slower now, however I think that we are better safe than sorry
There was a problem hiding this comment.
I'm inclined to extend testing even more by enabling both variants
Please, don't. This is pre-commit testing. I believe it's better to run such tests at least in post-commit.
There was a problem hiding this comment.
There will always remain a little risk that adding -d3 will change behavior -- IMO very small but still present.
My recommendation would be to:
-
- Remove suspicious/risky statements
if (debug_level < XYZ ) return;(in a separate merge request).
- Remove suspicious/risky statements
-
- Pick one verbosity level for the tests that run in the CI on per-commit basis (I prefer
-d3slightly more but don't feel strongly).
- Pick one verbosity level for the tests that run in the CI on per-commit basis (I prefer
-
- Optionally, if you like: Run the tests with the other verbosity level, too, but less frequently. For example:
- When comitting/merging to the main
riscvbranch. - Or every night on the main
riscvbranch (schedule options in Github Actions). - Or similar
-
- Optionally, alternate approach: Use different logging level on different targets. This would at least introduce more variability into the testing.
There was a problem hiding this comment.
Please, don't. This is pre-commit testing. I believe it's better to run such tests at least in post-commit.
I don't believe we should reduce amount when creating a commit in open source. We already had few regressions that could be detected by a more rigorous testing. This commit is a direct response on one of such regression. I don't mind waiting 40 minutes instead of 20 to get testing results. I don't think this is a bit problem.
Remove suspicious/risky statements if (debug_level < XYZ ) return; (in a separate merge request)
The thing is it is not that simple and may require a significant effort. Things are getting more complicated since the decision to use such approach was promoted in upstream several months ago. So unless the policy regarding such condition is changed in upstream - the issue shall persist. But we definitely should revise existing codebase regarding the matter.
Pick one verbosity level for the tests that run in the CI on per-commit basis (I prefer -d3 slightly more but don't feel strongly).
My pick would be -d3 too since usually this logging level allows to debug test.
Optionally, if you like: Run the tests with the other verbosity level, too, but less frequently.
I'll investigate this . Thanks for the idea.
Optionally, alternate approach: Use different logging level on different targets. This would at least introduce more variability into the testing.
As for messing up with different logging levels, I believe this may be an over complication. However, your suggestion regarding splitting test into multiple job may help with this one.
| --remotelogfile-enable \ | ||
| --server_cmd "$GITHUB_WORKSPACE/src/openocd -d3" | ||
|
|
||
| - name: Run Spike64-2-hwthread Tests |
There was a problem hiding this comment.
Now that the number of of tests has increased, it may be worth to split them to multiple jobs that would then run in parallel, leading to overall shorter test time.
There was a problem hiding this comment.
yes, this is definitely a way to go. Will investigate how to do this.
There was a problem hiding this comment.
It looks to me that the current job can be split to multiple parallel jobs quite easily using strategy-matrix feature of Github Actions.
Here is a concrete example: https://github.com/HonzaMat/PyOpenocdClient/blob/main/.github/workflows/integration_tests.yml#L7
No description provided.