gh-raspberrypi/pico-sdk

Watch

Fork 0

pioasm: Proposal to add tests and coverage report #2789

Open

gonzalolarralde wants to merge 25 commits from gonzalolarralde/gonzalolarralde/add-pioasm-tests into develop

gonzalolarralde commented

2026-01-09 01:24:17 -06:00

(Migrated from github.com)

Hello 👋

I’m working on a small PIO compiler as a side project and needed a validation suite to ensure output alignment across implementations. This PR adds a snapshot-style test suite for pioasm that exercises most of the functionality by running real pioasm invocations and comparing captured output. The large diff is mostly test data rather than harness code.

Goals:

Focus on public behavior (CLI output) instead of internal APIs.
Keep tests easy to update: when expected output changes, regenerate snapshots or fix the underlying issue.
Keep compiler code changes minimal.

Coverage:

~84% line coverage under tools/pioasm/ (excluding generated files)
69.2% overall including generated parser

Notes:

pioasm_core_coverage_test.cc embeds a small command parser to run the test suite inside a cc_test. This avoids modifying compiler internals. I can take a different direction on this.
While working on this testcases detected a crash addressed in #2854.

I used LLMs to help generate test cases and harness code, but reviewed them for coherence and relevance.

Please let me know if this doesn’t align with the maintainers’ testing direction. I’m happy to adjust the approach, and completely understand if the decision is not to merge this PR.

Thanks for taking the time to review this!

Hello 👋 I’m working on a small PIO compiler as a side project and needed a validation suite to ensure output alignment across implementations. This PR adds a snapshot-style test suite for pioasm that exercises most of the functionality by running real pioasm invocations and comparing captured output. The large diff is mostly test data rather than harness code. Goals: - Focus on public behavior (CLI output) instead of internal APIs. - Keep tests easy to update: when expected output changes, regenerate snapshots or fix the underlying issue. - Keep compiler code changes minimal. Coverage: - ~84% line coverage under tools/pioasm/ (excluding generated files) - 69.2% overall including generated parser <img width="1264" height="763" alt="Screenshot 2026-01-09 at 4 12 28 AM" src="https://github.com/user-attachments/assets/31a21ce7-b20c-48c1-b685-65169039d818" /> Notes: - pioasm_core_coverage_test.cc embeds a small command parser to run the test suite inside a cc_test. This avoids modifying compiler internals. I can take a different direction on this. - While working on this testcases detected a crash addressed in #2854. I used LLMs to help generate test cases and harness code, but reviewed them for coherence and relevance. Please let me know if this doesn’t align with the maintainers’ testing direction. I’m happy to adjust the approach, and completely understand if the decision is not to merge this PR. Thanks for taking the time to review this!

lurch commented

2026-03-08 10:40:20 -05:00

(Migrated from github.com)

This also fixes two segfaults by treating irq prev/next ... rel as syntax errors and aborting cleanly, this was catched while working on the testcases.

Maybe it's worth splitting that out to a separate PR, so that we can pull in the fix independently of your piosam test suite?

> This also fixes two segfaults by treating irq prev/next ... rel as syntax errors and aborting cleanly, this was catched while working on the testcases. Maybe it's worth splitting that out to a separate PR, so that we can pull in the fix independently of your `piosam` test suite?

👍 1

gonzalolarralde commented

2026-03-08 13:20:09 -05:00

(Migrated from github.com)

@lurch thanks for the feedback! I just created #2854, if/when merged I will rollback the change here. I want to make sure the tests still pass.

👍 1

gonzalolarralde commented

2026-03-08 13:25:33 -05:00

(Migrated from github.com)

This commit shows the process to update the tests after a change introduced in the compiler that is valid and accepted starts generating a new ouput.

In this case running the test harness ./run_tests.py failed most of the tests as a new syntax alias was added to extend support to constexpr when using cpp (#2651). The process to accept the new output of the test commands is to just review and commit the new test files with the changes applied, which is done automatically by running ./run_tests.py --overwrite

[This commit](https://github.com/raspberrypi/pico-sdk/pull/2789/changes/98231df538653e95729badb36eb27ad86a73cd05) shows the process to update the tests after a change introduced in the compiler that is valid and accepted starts generating a new ouput. In this case running the test harness `./run_tests.py` failed most of the tests as a new syntax alias was added to extend support to `constexpr` when using cpp (#2651). The process to accept the new output of the test commands is to just review and commit the new test files with the changes applied, which is done automatically by running `./run_tests.py --overwrite`

kilograham commented

2026-06-15 19:51:20 -05:00

(Migrated from github.com)

thanks for this - as it is a big PR will consider for next release

gonzalolarralde commented

2026-06-15 20:27:19 -05:00

(Migrated from github.com)

@kilograham Completely reasonable! Also as this change doesn't include any runtime change it should be fine for any version where it makes sense.

If you think there's a way to make this more approachable, like breaking it up in multiple PRs, I can def. work on that if there's alignment with the overall direction.

Maybe a good start would be just putting up the harness for review, and then tests can be brought individually based on possible value.

In any case, no rush. It was just an experiment that I figured could be helpful.

@kilograham Completely reasonable! Also as this change doesn't include any runtime change it should be fine for any version where it makes sense. If you think there's a way to make this more approachable, like breaking it up in multiple PRs, I can def. work on that if there's alignment with the overall direction. Maybe a good start would be just putting up the harness for review, and then tests can be brought individually based on possible value. In any case, no rush. It was just an experiment that I figured could be helpful.

This pull request can be merged automatically.

You are not authorized to merge this pull request.

View command line instructions

Checkout

From your project repository, check out a new branch and test the changes.

git fetch -u origin 'gonzalolarralde/gonzalolarralde/add-pioasm-tests:gonzalolarralde/gonzalolarralde/add-pioasm-tests'

git switch 'gonzalolarralde/gonzalolarralde/add-pioasm-tests'

Merge

Merge the changes and update on Forgejo.

Warning: The "Autodetect manual merge" setting is not enabled for this repository, you will have to mark this pull request as manually merged afterwards.

git switch 'develop'

git merge --no-ff 'gonzalolarralde/gonzalolarralde/add-pioasm-tests'

git switch 'gonzalolarralde/gonzalolarralde/add-pioasm-tests'

git rebase 'develop'

git switch 'develop'

git merge --ff-only 'gonzalolarralde/gonzalolarralde/add-pioasm-tests'

git switch 'gonzalolarralde/gonzalolarralde/add-pioasm-tests'

git rebase 'develop'

git switch 'develop'

git merge --no-ff 'gonzalolarralde/gonzalolarralde/add-pioasm-tests'

git switch 'develop'

git merge --squash 'gonzalolarralde/gonzalolarralde/add-pioasm-tests'