For discussion: A way of improving regression/integration testing for ASL snippets by martin-hughes · Pull Request #269 · rust-osdev/acpi

martin-hughes · 2026-03-07T15:43:01Z

This PR is very much a proof of concept for improving testing of how ASL snippets are handled. It provides a black-box testing rig for ASL scripts that checks acpi calls the handler with the correct commands and values.

I don't really think it's ready for merging, it's mostly for comment / discussion at this time.

This idea came about because I was struggling to see what was happening with Store commands, and I see that there's only limited regression testing in general

aml_tester is useful for what it does, but it obviously doesn't let you check that the outputs from acpi are what you'd expect - only that the parser can handle the script somehow.

In terms of design notes, I included the ASL snippets in rust files for a few reasons:

It keeps the inputs (ASL) and outputs (expected ACPI Handler calls) in one file - for ease of maintenance
The expected outputs can be written in Rust and checked by the IDE
This design plays nicely with cargo test and/or IDE testing functionality (no need for cargo run_tests)

I spent some time wondering if the tests would be too brittle - but I think the Handler interface is not likely to change too often, and my feeling is that it's unlikely that changes to the parser would cause big changes to the sequence of Handler calls.

Thoughts anyone?

martin-hughes · 2026-03-07T15:43:33Z

tests/basic.rs

+mod test_infra;
+
+#[test]
+#[ignore]


Ignore this test for now and concentrate on the next one.

martin-hughes · 2026-03-07T15:44:03Z

tests/basic.rs

+    test_infra::run_aml_test(AML, handler);
+}
+
+#[test]


This test demonstrates the kind of style I was thinking of

martin-hughes · 2026-03-07T15:46:43Z

tools/aml_test_tools/src/lib.rs

This file is basically a cut-paste of functions from main.rs that are useful in my new test system. This clearly isn't very neat, probably best to extract to a small asl_testing crate or similar.

IsaacWoods · 2026-03-08T16:33:32Z

I'm a big fan of this idea in general, and quite like the approach you've taken here! It was always my intention to have regression testing along these lines at some point, but I never got around to building it out, so thanks for working on this.

I guess the only downside to keeping the ASL inline with the test is a lack of syntax highlighting for those who have it set up. That's probably not the biggest loss with small snippets like most of these tests are likely to be I guess.

I'm not sure what I think about the overall library layout; I think we could likely have a neater approach by separating the compilation of the ASL and testing into a little library and then use it from a Rust-based regression test-suite and also a command-line tool for compiling and running a provided blob (which is still useful for eg testing some hardware's extracted tables). I think that might be what you're already suggesting? In which case definitely green light on that.

IsaacWoods · 2026-03-08T16:34:25Z

Cargo.toml

 pci_types = { version = "0.10.0", public = true }
 byteorder = { version = "1.5.0", default-features = false }

+[dev-dependencies]


I might be mistaken but do these need to be dev-dependencies on the top-level acpi crate? Or dependencies on the aml_test_tools crate?

Dependencies on the sub-crate almost certainly. I'll flesh it out and update the PR.

IsaacWoods · 2026-03-08T16:35:16Z

tests/basic.rs

+    test_infra::run_aml_test(AML, handler);
+}
+
+#[test]


IsaacWoods · 2026-03-08T16:36:39Z

tools/aml_test_tools/src/handlers/std_test_handler.rs

+
+pub type Command = (Check, Response);
+
+pub fn construct_std_handler(commands: Vec<Command>) -> impl Handler {


I like the concept of "stacking" handlers but wonder if there is utility in separating checking for actions in the right order and what the handlers responses should be? I feel a single handler that does both might be easier to understand.

I spent a bit of time wondering about this before writing it. The example I thought of is that I might want to write a handler that stores all the writes (e.g. in a map of IO ports -> written value) and then reads them back on the relevant read. Which might be useful for longer or more complex tests. We might still want to check which handler functions are called but with this new innards (i.e. the constructor call would be CheckCommandHandler::new(c, StoredResponsesHandler::new(r)) or similar)

Plus SRP, although I might have gone a bit overboard in this case 😆

If you're not a fan just pop a comment here and I'll merge them into one handler.

IsaacWoods · 2026-03-08T16:38:56Z

tests/test_infra/check_cmd_handler.rs

+    }
+
+    fn check_command(&self, command: AcpiCommands) {
+        let next_command_idx = self.next_command_idx.load(Relaxed);


This doesn't matter in this case really, but for correctness I feel this should do the fetch_add here

Good shout.

martin-hughes · 2026-03-08T20:40:06Z

Thanks for taking a look @IsaacWoods 😊 I'll work on it some more and post an update when I can.

I guess the only downside to keeping the ASL inline with the test is a lack of syntax highlighting for those who have it set up. That's probably not the biggest loss with small snippets like most of these tests are likely to be I guess.

Interesting point that I hadn't thought of... I agree that most snippets should be pretty simple. The only mitigation I can think of for longer scripts would be to use include_str! to include more complex ASL files directly. This might actually suit well - longer snippets would make it harder to explore the tests anyway.

I think that might be what you're already suggesting? In which case definitely green light on that.

Pretty much, yes - just laziness had prevailed for this first draft 😆.

Cheers again.

`run_test_for_file` and `run_test_for_string` both now return `TestResult`

martin-hughes · 2026-03-09T15:05:05Z

Updated as above, except I haven't merged the check/response handlers - do you want me to, or was my explanation OK?

The only dev-dependencies left on the main crate are the ones used by the tests themselves.

Ready for another look @IsaacWoods 😊

Having to create `Check` and `Response` items for each test is a bit laborious, so this simplifies things whilst keeping the SRP-ness of the two Handlers / allowing for future flexibility.

martin-hughes · 2026-03-12T10:58:44Z

I've made a few ergonomic improvements this morning that reduce the burden to the end user of using the standard test handler, whilst keeping the flexibility to easily add other handlers/combinations of handlers in future.

martin-hughes · 2026-03-13T21:21:43Z

tools/aml_test_tools/src/lib.rs

+    let output_name = format!("{}.aml", output_stem);
+    let output_full_name = dir.path().join(output_name.clone());
+
+    let new_asl = asl.replace("%FN%", output_name.as_str());


I only just realised you don't need to specify a filename as the first parameter of DefinitionBlock 🤦. Want me to strip this feature out?

Hm, does iasl infer the correct filename for the result if not provided? If that works consistently then I have no issue utilising it, but possible it differs on iasl version or something - no strong feelings either way.

Good question - it seems to always infer it, but I guess it could change without warning. I see you left it in, and I think that would probably have been my preference in the end anyway.

Cheers for your comments on this whole PR!

IsaacWoods · 2026-03-14T10:50:29Z

This is really fantastic work, thanks @martin-hughes!
This looks like it'll be useful for my next steps of working out our semantics woes re references (will get back to you on your related issues shortly) so let's get this merged here, and we can work out details re multiple tests in future PRs.

Cheers!

--- Edit ---
Your reasoning on the split handlers makes sense - I can see future use for e.g. recording a problematic firmwares responses somehow and utilising a sort-of replay handler or better modelling of I/O ports or a PCIe bus if we want to get fancier. Thanks.

martin-hughes commented Mar 7, 2026

View reviewed changes

tests/basic.rs Outdated

mod test_infra;

#[test]

#[ignore]

Copy link

Contributor Author

martin-hughes Mar 7, 2026

Choose a reason for hiding this comment

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

Ignore this test for now and concentrate on the next one.

martin-hughes commented Mar 7, 2026

View reviewed changes

IsaacWoods reviewed Mar 8, 2026

View reviewed changes

martin-hughes added 5 commits March 8, 2026 20:43

A possible way to regression test ASL snippets

a59f12a

Delete distracting ASL files

dd2b2c9

Refactor common functions to new crate

37346fc

Make test runner interfaces the same

a4099c6

`run_test_for_file` and `run_test_for_string` both now return `TestResult`

Add a few tests as a demo

5743dcb

martin-hughes force-pushed the possible-test-infra branch from 2a3216d to 5743dcb Compare March 9, 2026 15:02

martin-hughes marked this pull request as ready for review March 9, 2026 15:05

martin-hughes mentioned this pull request Mar 11, 2026

Implement Index and Bank fields #274

Open

martin-hughes added 2 commits March 12, 2026 10:29

Ergonomic improvements

b28f449

Having to create `Check` and `Response` items for each test is a bit laborious, so this simplifies things whilst keeping the SRP-ness of the two Handlers / allowing for future flexibility.

Documentation updates

2455763

Not all tests actually pass...

eb1ea7b

martin-hughes commented Mar 13, 2026

View reviewed changes

martin-hughes mentioned this pull request Mar 13, 2026

aml_tester only supports one table per AML file. #275

Open

IsaacWoods merged commit 21e8039 into rust-osdev:main Mar 14, 2026
5 checks passed

martin-hughes deleted the possible-test-infra branch March 14, 2026 21:40


		pub type Command = (Check, Response);

		pub fn construct_std_handler(commands: Vec<Command>) -> impl Handler {

Uh oh!

Conversation

martin-hughes commented Mar 7, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

IsaacWoods commented Mar 8, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

martin-hughes commented Mar 8, 2026

Uh oh!

martin-hughes commented Mar 9, 2026

Uh oh!

martin-hughes commented Mar 12, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

IsaacWoods commented Mar 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

IsaacWoods commented Mar 14, 2026 •

edited

Loading