Add a test that an odd capacity `BufEncoder` does not compile #157

jamillambert · 2025-02-19T16:42:30Z

An even capacity BufEncoder is now enforced and should be tested to make sure it does not compile if an odd capacity is used.

Adds a crate in tests that uses an odd capacity BufEncoder.

Adds a CI test that passes if the odd capacity crate fails to compile.

Close #155

Kixunil · 2025-02-19T17:11:24Z

tests/compiletests.rs

+    config.src_base = PathBuf::from("tests/compiletest");
+    config.target_rustcflags = Some("-L target/debug/deps".to_string());
+    compiletest::run_tests(&config);
+}


Looks quite complicated. Why not simply have a new CI step to run if cargo check --something; then exit 1; fi?

I wasn't sure about adding a CI step just for this one, should be simple, test. But it does work, and there might be reasons in the future to add to the compile tests?

I think it's probably worthwhile to do this in a general way (modifying CI). If we had the means to add compile-tests I'll bet we'd add a lot of them.

+1 this would be useful for the api test files. And also for corepc.

I prefer not to take dev dependency if the same can be done in CI reasonably simply.

tcharding · 2025-02-21T01:28:04Z

Cool, looking good. I think this should be called odd-capacity instead of compiletest because we can only ever have a single test in it (since one cannot prove two build failures at the same time). And for that reason perhaps inline the function into main.

FTR I changed the 3 to a 4 and the crate builds - to check for no false positives. This crate risks going stale consider this:

We change the import path of BufEncode - now the test crate does not build because of that
We change the BufEncode type accidentally so that odd value builds - now the test does not catch it.

jamillambert · 2025-02-21T09:07:47Z

I have changed it so that there are two compile checks now. First it checks that an even capacity compiles, then it checks that the odd capacity doesn't. The only difference between the two is changing the capacity from 4 to 3.

I also moved them to a sub directory in tests called compiletests so it is a bit tidier, especially if more are going to be added.

apoelstra · 2025-02-23T14:51:44Z

Can you add a further subdirectory level tests/compiletest/pass/ and .../fail/ and have the script use the directory names to figure out whether this should be a pass or fail test?

jamillambert · 2025-02-24T09:56:53Z

I have changed the script to be general using the two sub directories pass/ and fail/.

I added the BufEncoder capacity evenness check as a separate commit.

tcharding · 2025-02-25T02:42:53Z

Looks good, few bash things specific to the rust-bitcoin org

We use #!/usr/bin/env bash so that Nix users can run the script (they don't typically have /bin/bash I'm told)
Tend to use set -euo pipefail unless some reason not to
Try to lint scripts with shellcheck foo.sh https://www.shellcheck.net/
Tend to use REPO_DIR=$(git rev-parse --show-toplevel) to get at the repo directory
If you feel like it (but its mainly only me that does it) you can use a bunch of helper functions that were originally written by the Rust project a long time ago. They are in various shell scripts around the place, bitcoin/contrib/check-for-api-changes.rs has them)

tcharding · 2025-02-25T02:46:12Z

Can you put a - separator in compiletests directory please mate. Based on that ~~famous~~ made-up saying "Uniformity is close to Godliness"

jamillambert · 2025-02-25T11:57:41Z

Made the changes to the shell script and directory name.

Kixunil · 2025-02-25T12:26:52Z

contrib/compile-tests.sh

+    for dir in pass/*; do
+        if [ -d "$dir" ]; then
+            pushd "$dir" > /dev/null
+            if ! cargo run > /dev/null 2>&1; then


The test says "compile" but it uses run. Also we should keep the output here.

Changed to build. I have also made it so that it only outputs if the test failed, otherwise for all the shouldn't compile cases there will be a list of compile errors when everything is working as intended.

This is the pass branch though.

Kixunil · 2025-02-25T12:28:21Z

contrib/compile-tests.sh

+    if [ "$all_passed" = true ]; then
+        return 0
+    else
+        return 1


Perhaps just define result = 0 at the beginning and assign it to 1 on fail and just return "$result" to get rid of if.

Thanks, this is a nicer way of doing it. I have changed it as suggested.

Kixunil · 2025-02-25T12:28:47Z

contrib/compile-tests.sh

+    for dir in fail/*; do
+        if [ -d "$dir" ]; then
+            pushd "$dir" > /dev/null
+            if cargo run > /dev/null 2>&1; then


Also it should be build, not run.

Kixunil · 2025-02-25T12:29:04Z

contrib/compile-tests.sh

+    if [ "$all_failed" = true ]; then
+        return 0
+    else
+        return 1


Same as above.

Kixunil · 2025-02-25T14:16:45Z

contrib/compile-tests.sh

+    for dir in pass/*; do
+        if [ -d "$dir" ]; then
+            pushd "$dir" > /dev/null
+            output=$(cargo build 2>&1)


This shouldn't be needed but I guess it's OK. I don't expect the output to be too large.

Kixunil · 2025-02-25T14:18:04Z

contrib/compile-tests.sh

+            pushd "$dir" > /dev/null
+            output=$(cargo build 2>&1)
+            if [ $? -eq 0 ]; then
+                echo "$output"


If compilation succeeded and it shouldn't there will be no useful information printed by cargo. I'd rather get rid of it.

I have removed the cargo output for this case

jamillambert · 2025-02-25T14:26:34Z

Looking at the script again I noticed the two functions are now basically identical. I have refactored it to have a single function that handles both cases.

Create a script `contrib/compile-tests.sh` the checks that all crates in `tests/compiletests/` do compile if in `pass/` and fail to compile if in `fail/` Add a CI step to run the script

Add two compile test crates that are identical except for the capacity of `BufEncoder`. Odd capacity in `fail/` that must not compile and even capacity in `pass/` that must compile.

Kixunil · 2025-02-25T21:28:39Z

contrib/compile-tests.sh

+            echo "Compiling $dir"
+            output=$(cargo build 2>&1)
+            if [ "$?" -ne "$expected_status" ]; then
+                if [ "$expected_status" -eq 0 ]; then


Why have another argument if it can be inferred from the first one and you're not comparing them anyway?

I started to change it, but it's more flexible the way it is. I'm not sure we need to but it's possible to check directories other than pass/ and fail/ by passing a different argument to the function.

But if you prefer I can hard code the directory names and exit codes.

Too many possibilities that we're not using are just complicating the code. Honestly, I think even having the pass option is a stretch (normal tests do that already!) but I understand it's for the symmetry since there are related test (one for 3, other 4) so I'm accepting it. I was really expecting this to be one line in the workflow file.

tcharding

ACK 853f493

Kixunil reviewed Feb 19, 2025

View reviewed changes

jamillambert force-pushed the 0219-compile-test branch 3 times, most recently from 7e295d7 to 3394da3 Compare February 19, 2025 21:18

jamillambert marked this pull request as ready for review February 19, 2025 21:19

jamillambert force-pushed the 0219-compile-test branch 2 times, most recently from ed6b613 to 5a70ac4 Compare February 21, 2025 09:04

jamillambert force-pushed the 0219-compile-test branch from 5a70ac4 to 3e23c88 Compare February 24, 2025 09:55

jamillambert force-pushed the 0219-compile-test branch from 3e23c88 to 2751fa4 Compare February 24, 2025 09:59

jamillambert force-pushed the 0219-compile-test branch from 2751fa4 to 24059d2 Compare February 25, 2025 11:57

Kixunil requested changes Feb 25, 2025

View reviewed changes

jamillambert force-pushed the 0219-compile-test branch from 24059d2 to d91a533 Compare February 25, 2025 13:38

Kixunil reviewed Feb 25, 2025

View reviewed changes

jamillambert force-pushed the 0219-compile-test branch from d91a533 to 82918e6 Compare February 25, 2025 14:24

jamillambert added 2 commits February 25, 2025 14:34

Add CI check for compile tests

d8b202c

Create a script `contrib/compile-tests.sh` the checks that all crates in `tests/compiletests/` do compile if in `pass/` and fail to compile if in `fail/` Add a CI step to run the script

Add compile test for odd capacity BufEncoder

853f493

Add two compile test crates that are identical except for the capacity of `BufEncoder`. Odd capacity in `fail/` that must not compile and even capacity in `pass/` that must compile.

jamillambert force-pushed the 0219-compile-test branch from 82918e6 to 853f493 Compare February 25, 2025 14:36

Kixunil reviewed Feb 25, 2025

View reviewed changes

tcharding approved these changes Feb 26, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a test that an odd capacity `BufEncoder` does not compile #157

Add a test that an odd capacity `BufEncoder` does not compile #157

jamillambert commented Feb 19, 2025 •

edited

Loading

Kixunil Feb 19, 2025

jamillambert Feb 19, 2025

apoelstra Feb 19, 2025

tcharding Feb 19, 2025

Kixunil Feb 20, 2025 •

edited

Loading

tcharding commented Feb 21, 2025

jamillambert commented Feb 21, 2025

apoelstra commented Feb 23, 2025 •

edited

Loading

jamillambert commented Feb 24, 2025

tcharding commented Feb 25, 2025

tcharding commented Feb 25, 2025 •

edited

Loading

jamillambert commented Feb 25, 2025

Kixunil Feb 25, 2025

jamillambert Feb 25, 2025

Kixunil Feb 25, 2025

Kixunil Feb 25, 2025

jamillambert Feb 25, 2025

Kixunil Feb 25, 2025

jamillambert Feb 25, 2025

Kixunil Feb 25, 2025

jamillambert Feb 25, 2025

Kixunil Feb 25, 2025

Kixunil Feb 25, 2025

jamillambert Feb 25, 2025

jamillambert commented Feb 25, 2025

Kixunil Feb 25, 2025

jamillambert Feb 26, 2025

Kixunil Feb 26, 2025

tcharding left a comment

Add a test that an odd capacity BufEncoder does not compile #157

Are you sure you want to change the base?

Add a test that an odd capacity BufEncoder does not compile #157

Conversation

jamillambert commented Feb 19, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kixunil Feb 20, 2025 • edited Loading

Choose a reason for hiding this comment

tcharding commented Feb 21, 2025

jamillambert commented Feb 21, 2025

apoelstra commented Feb 23, 2025 • edited Loading

jamillambert commented Feb 24, 2025

tcharding commented Feb 25, 2025

tcharding commented Feb 25, 2025 • edited Loading

jamillambert commented Feb 25, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jamillambert commented Feb 25, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tcharding left a comment

Choose a reason for hiding this comment

Add a test that an odd capacity `BufEncoder` does not compile #157

Add a test that an odd capacity `BufEncoder` does not compile #157

jamillambert commented Feb 19, 2025 •

edited

Loading

Kixunil Feb 20, 2025 •

edited

Loading

apoelstra commented Feb 23, 2025 •

edited

Loading

tcharding commented Feb 25, 2025 •

edited

Loading