Skip to content

Commit 85f6788

Browse files
azerupiazerupi
authored
Make generated order deterministic and add snapshot testing (#387)
* Make generated order deterministic and add snapshot testing This changes the hashmaps to indexmaps that preserve order of insertion when iterating over them. This makes the generated code order deterministic and consistent with the definition file. This also adds snapshot testing to ensure that the generated code remains consistent across changes or makes it very obvious when it does change. * Add documentation for snapshot tests * Switch to BTreeMap and remove indexmap dependency
1 parent 163961c commit 85f6788

File tree

10 files changed

+962
-4
lines changed

10 files changed

+962
-4
lines changed

‎mavlink-bindgen/Cargo.toml‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -38,3 +38,7 @@ cli = ["dep:clap", "dep:clap_lex", "dep:clap_builder", "dep:anstyle", "dep:ansty
3838
arbitrary = ["dep:arbitrary", "dep:rand"]
3939
emit-extensions = []
4040
emit-description = ["dep:regex"]
41+
42+
[dev-dependencies]
43+
insta = { version = "1.39.0", features = ["glob"] }
44+
tempfile = "3.10.1"

‎mavlink-bindgen/README.md‎

Lines changed: 74 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -101,3 +101,77 @@ pub use mavlink_core::*;
101101
Since each dialect is locked behind a feature flag these need to be enabled for the dialects to become available when using the generated code.
102102

103103
This approach is used by the `mavlink` crate see its build script for an example.
104+
105+
## Contributing
106+
107+
### Snapshot tests
108+
109+
This crate uses snapshot tests to guard against subtle changes to the generated code.
110+
The tests generate the formatted MAVLink Rust code from small MAVLink definition files under `tests/definitions/`
111+
and assert that the output hasn't changed. If the output changed the tests will fail. Snapshots can easily be updated
112+
with the new changes if they are valid.
113+
114+
Performing snapshot testing has two main purposes, the first is to avoid having changes to the generated code
115+
go unnoticed. Any change to the generated code will be very obvious with the snapshot tests. The second
116+
purpose is that by checking in the snapshots we have examples of generated code and changes to generated
117+
code are included in the review diff, making reviews easier.
118+
119+
#### Layout
120+
121+
- `tests/definitions/`: Definition files including MAVLink messages. Keeping the amount of messages per definition file
122+
to a minimum will make it easier to review the output in the snapshots.
123+
- `tests/e2e_snapshots.rs`: This is where the tests live. It invokes the generator for each XML definition file and snapshots all emitted `.rs` files.
124+
- `tests/snapshots/`: The committed snapshot files created by `insta` are located here. Only the `.snap` files should be commited, `.snap.new` files
125+
are created automatically when the tests detect diverging output. They have to be reviewed and either the generator has to be fixed or the
126+
snapshots have to be updated.
127+
128+
#### Running tests
129+
130+
Run all tests (including snapshots):
131+
132+
```bash
133+
cargo test
134+
```
135+
136+
On the first run or after codegen changes, new snapshots will be created with a `.snap.new` suffix. Review and accept them if the changes are intended.
137+
138+
#### Reviewing and accepting snapshots
139+
140+
For a better workflow, install `cargo-insta` and use it to collect and review updates interactively. See the Insta quickstart for details: [Insta quickstart](https://insta.rs/docs/quickstart/).
141+
142+
```bash
143+
# Install cargo-insta from crates.io
144+
cargo install cargo-insta
145+
```
146+
147+
After running `cargo test` and snapshot tests have failed:
148+
```bash
149+
# interactively review and accept/reject changes
150+
cargo insta review
151+
```
152+
153+
Alternatively, if you don't want to install `cargo-insta` you can control updates via the `INSTA_UPDATE` environment variable:
154+
155+
```bash
156+
# do not update snapshots (useful for CI-like checks)
157+
INSTA_UPDATE=no cargo test -p mavlink-bindgen
158+
159+
# overwrite all existing snapshots with new results
160+
INSTA_UPDATE=always cargo test -p mavlink-bindgen
161+
162+
# write only new snapshots (existing ones stay unchanged)
163+
INSTA_UPDATE=new cargo test -p mavlink-bindgen
164+
```
165+
166+
#### Adding a new snapshot test
167+
168+
1. Create a minimal XML in `tests/definitions/`, e.g. `foo.xml`. Prefer one message or a tiny set of enums/messages to keep snapshots small and stable.
169+
2. Run the tests: `cargo test -p mavlink-bindgen` (or `cargo insta test`).
170+
3. Review and accept the new snapshots: `cargo insta review`.
171+
172+
The test harness automatically discovers generated `.rs` files for each XML and creates one snapshot per file (e.g. `[email protected]`).
173+
174+
#### Determinism and formatting
175+
176+
The generator strives to emit items in a deterministic order.
177+
If you change code generation intentionally, expect corresponding snapshot updates. Review the diffs carefully for unintended regressions.

‎mavlink-bindgen/src/parser.rs‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
use crc_any::CRCu16;
22
use std::cmp::Ordering;
3-
use std::collections::hash_map::Entry;
4-
use std::collections::{HashMap, HashSet};
3+
use std::collections::btree_map::Entry;
4+
use std::collections::{BTreeMap, HashSet};
55
use std::default::Default;
66
use std::fs::File;
77
use std::io::{BufReader, Write};
@@ -41,8 +41,8 @@ lazy_static! {
4141
#[derive(Debug, PartialEq, Clone, Default)]
4242
#[cfg_attr(feature = "serde", derive(Serialize, Deserialize))]
4343
pub struct MavProfile {
44-
pub messages: HashMap<String, MavMessage>,
45-
pub enums: HashMap<String, MavEnum>,
44+
pub messages: BTreeMap<String, MavMessage>,
45+
pub enums: BTreeMap<String, MavEnum>,
4646
}
4747

4848
impl MavProfile {

‎mavlink-bindgen/tests/definitions/heartbeat.xml‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,12 @@
1+
<mavlink>
2+
<messages>
3+
<message id="0" name="HEARTBEAT">
4+
<field type="uint32_t" name="custom_mode">Custom mode</field>
5+
<field type="uint8_t" name="type">Type</field>
6+
<field type="uint8_t" name="autopilot">Autopilot</field>
7+
<field type="uint8_t" name="base_mode">Base mode</field>
8+
<field type="uint8_t" name="system_status">System status</field>
9+
<field type="uint8_t_mavlink_version" name="mavlink_version">Mavlink version</field>
10+
</message>
11+
</messages>
12+
</mavlink>

‎mavlink-bindgen/tests/definitions/parameters.xml‎

Lines changed: 107 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,107 @@
1+
<mavlink>
2+
<enums>
3+
<enum name="MAV_PARAM_TYPE">
4+
<description>Specifies the datatype of a MAVLink parameter.</description>
5+
<entry value="1" name="MAV_PARAM_TYPE_UINT8">
6+
<description>8-bit unsigned integer</description>
7+
</entry>
8+
<entry value="2" name="MAV_PARAM_TYPE_INT8">
9+
<description>8-bit signed integer</description>
10+
</entry>
11+
<entry value="3" name="MAV_PARAM_TYPE_UINT16">
12+
<description>16-bit unsigned integer</description>
13+
</entry>
14+
<entry value="4" name="MAV_PARAM_TYPE_INT16">
15+
<description>16-bit signed integer</description>
16+
</entry>
17+
<entry value="5" name="MAV_PARAM_TYPE_UINT32">
18+
<description>32-bit unsigned integer</description>
19+
</entry>
20+
<entry value="6" name="MAV_PARAM_TYPE_INT32">
21+
<description>32-bit signed integer</description>
22+
</entry>
23+
<entry value="7" name="MAV_PARAM_TYPE_UINT64">
24+
<description>64-bit unsigned integer</description>
25+
</entry>
26+
<entry value="8" name="MAV_PARAM_TYPE_INT64">
27+
<description>64-bit signed integer</description>
28+
</entry>
29+
<entry value="9" name="MAV_PARAM_TYPE_REAL32">
30+
<description>32-bit floating-point</description>
31+
</entry>
32+
<entry value="10" name="MAV_PARAM_TYPE_REAL64">
33+
<description>64-bit floating-point</description>
34+
</entry>
35+
</enum>
36+
</enums>
37+
<messages>
38+
<message id="20" name="PARAM_REQUEST_READ">
39+
<description>Request to read the onboard parameter with the param_id string id. Onboard
40+
parameters are stored as key[const char*] -&gt; value[float]. This allows to send a
41+
parameter to any other component (such as the GCS) without the need of previous
42+
knowledge of possible parameter names. Thus the same GCS can store different
43+
parameters
44+
for different autopilots. See also https://mavlink.io/en/services/parameter.html for
45+
a
46+
full documentation of QGroundControl and IMU code.</description>
47+
<field type="uint8_t" name="target_system">System ID</field>
48+
<field type="uint8_t" name="target_component">Component ID</field>
49+
<field type="char[16]" name="param_id">Onboard parameter id, terminated by NULL if the
50+
length is less than 16 human-readable chars and WITHOUT null termination (NULL) byte
51+
if
52+
the length is exactly 16 chars - applications have to provide 16+1 bytes storage if
53+
the
54+
ID is stored as string</field>
55+
<field type="int16_t" name="param_index" invalid="-1">Parameter index. Send -1 to use
56+
the
57+
param ID field as identifier (else the param id will be ignored)</field>
58+
</message>
59+
<message id="21" name="PARAM_REQUEST_LIST">
60+
<description>Request all parameters of this component. After this request, all
61+
parameters
62+
are emitted. The parameter microservice is documented at
63+
https://mavlink.io/en/services/parameter.html</description>
64+
<field type="uint8_t" name="target_system">System ID</field>
65+
<field type="uint8_t" name="target_component">Component ID</field>
66+
</message>
67+
<message id="22" name="PARAM_VALUE">
68+
<description>Emit the value of a onboard parameter. The inclusion of param_count and
69+
param_index in the message allows the recipient to keep track of received parameters
70+
and
71+
allows him to re-request missing parameters after a loss or timeout. The parameter
72+
microservice is documented at https://mavlink.io/en/services/parameter.html</description>
73+
<field type="char[16]" name="param_id">Onboard parameter id, terminated by NULL if the
74+
length is less than 16 human-readable chars and WITHOUT null termination (NULL) byte
75+
if
76+
the length is exactly 16 chars - applications have to provide 16+1 bytes storage if
77+
the
78+
ID is stored as string</field>
79+
<field type="float" name="param_value">Onboard parameter value</field>
80+
<field type="uint8_t" name="param_type" enum="MAV_PARAM_TYPE">Onboard parameter type.</field>
81+
<field type="uint16_t" name="param_count">Total number of onboard parameters</field>
82+
<field type="uint16_t" name="param_index">Index of this onboard parameter</field>
83+
</message>
84+
<message id="23" name="PARAM_SET">
85+
<description>Set a parameter value (write new value to permanent storage).
86+
The receiving component should acknowledge the new parameter value by broadcasting a
87+
PARAM_VALUE message (broadcasting ensures that multiple GCS all have an up-to-date
88+
list
89+
of all parameters). If the sending GCS did not receive a PARAM_VALUE within its
90+
timeout
91+
time, it should re-send the PARAM_SET message. The parameter microservice is
92+
documented
93+
at https://mavlink.io/en/services/parameter.html.
94+
</description>
95+
<field type="uint8_t" name="target_system">System ID</field>
96+
<field type="uint8_t" name="target_component">Component ID</field>
97+
<field type="char[16]" name="param_id">Onboard parameter id, terminated by NULL if the
98+
length is less than 16 human-readable chars and WITHOUT null termination (NULL) byte
99+
if
100+
the length is exactly 16 chars - applications have to provide 16+1 bytes storage if
101+
the
102+
ID is stored as string</field>
103+
<field type="float" name="param_value">Onboard parameter value</field>
104+
<field type="uint8_t" name="param_type" enum="MAV_PARAM_TYPE">Onboard parameter type.</field>
105+
</message>
106+
</messages>
107+
</mavlink>

‎mavlink-bindgen/tests/e2e_snapshots.rs‎

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,38 @@
1+
use std::fs;
2+
use std::path::PathBuf;
3+
4+
use insta::{self, assert_snapshot, glob};
5+
use mavlink_bindgen::{format_generated_code, generate, XmlDefinitions};
6+
use tempfile::TempDir;
7+
8+
fn definitions_dir() -> PathBuf {
9+
PathBuf::from(env!("CARGO_MANIFEST_DIR"))
10+
.join("tests")
11+
.join("definitions")
12+
}
13+
14+
fn run_snapshot(def_file: &str) {
15+
let defs = definitions_dir();
16+
let tmp = TempDir::new().expect("tmp dir");
17+
let out_dir = tmp.path();
18+
19+
let xml = defs.join(def_file);
20+
let result = generate(XmlDefinitions::Files(vec![xml]), out_dir).expect("generate ok");
21+
22+
format_generated_code(&result);
23+
24+
glob!(out_dir, "**/*.rs", |path| {
25+
let contents = fs::read_to_string(path).expect("read generated file");
26+
assert_snapshot!(def_file, contents);
27+
})
28+
}
29+
30+
#[test]
31+
fn snapshot_heartbeat() {
32+
run_snapshot("heartbeat.xml");
33+
}
34+
35+
#[test]
36+
fn snapshot_parameters() {
37+
run_snapshot("parameters.xml");
38+
}

0 commit comments

Comments
 (0)