Pytest tests and CI #58

jonrebm · 2025-12-14T19:46:01Z

I've done some cleanup in the microcom codebase, and created a CI workflow to catch many of the encountered issues in the future.

These tests fail now, as long as the PRs resolving the issues are still open:

Pass many gcc diagnostics, follow some code style rules; see General code cleanup #55
Don't use uninitialized or freed memory, see microcom.c: Call cleanup routines only once #56
Don't access arrays out-of-bounds and test telnet.c to correctly handle command sequences at buffer boundaries, see telnet.c: rewrite command handling #57

The quiet switch shall silence all printing to stdout during normal operation, but leaves the interactive console and all error messages functional. Useful for automation such as in CI. Signed-off-by: Jonas Rebmann <[email protected]>

I set up those tests for the issues with the current telnet implementation. They can serve as regression tests after the issues have been resolved. Add them as the first integration tests. Add pythons bytecode cache files to gitignore. Signed-off-by: Jonas Rebmann <[email protected]>

- correct branch to main, which our default branch - fix dependency installation - invoke pytest - don't build an unneeded release tarball Signed-off-by: Jonas Rebmann <[email protected]>

This has proven to be useful to catch out-of-bounds access and I assume will prevent many cases of success by chance when testing microcom. Signed-off-by: Jonas Rebmann <[email protected]>

The rules are rather flexible so they should be enforcible treewide. If not, maybe there's a way to at least only enforce it on the sections or at least files that the commits touched? Signed-off-by: Jonas Rebmann <[email protected]>

Amongst others, this catches uninitialized memory access, which libasan does not. Signed-off-by: Jonas Rebmann <[email protected]>

hnez · 2025-12-16T13:32:41Z

microcom.c

+		case '?':
+			main_usage(1, "", "");
+			break;
+		case 'h':
+			main_usage(0, "", "");
+			break;
+		case 'v':
+			printf("%s\n", PACKAGE_VERSION);
+			exit(EXIT_SUCCESS);
+			break;
+		case 'p':
+			device = optarg;
+			break;
+		case 's':
+			current_speed = strtoul(optarg, NULL, 0);
+			break;
+		case 't':
+			telnet = 1;
+			hostport = optarg;
+			break;
+		case 'c':
+			can = 1;
+			interfaceid = optarg;
+			break;
+		case 'f':
+			opt_force = 1;
+			break;
+		case 'd':
+			debug = 1;
+			break;
+		case 'q':
+			quiet = 1;
+			break;
+		case 'l':
+			logfile = optarg;
+			break;
+		case 'o':
+			listenonly = 1;
+			break;
+		case 'a':
+			answerback = optarg;
+			break;
+		case 'e':
+			if (strlen(optarg) != 1) {
+				fprintf(stderr, "Option -e requires a single character argument.\n");
+				exit(EXIT_FAILURE);
+			}
+			escape_char = *optarg;
+			break;


This mixes a formatting change with a code change. It would be great to only have the

+ case 'q': + quiet = 1; + break;

In this commit.

I originally based this on all the three dependent branches and only haphazardly rebased to main to avoid making too large a pull request.
The diff here will be more meaningful once #55 is closed. I'm converting this pr to draft until then.

hnez · 2025-12-16T13:35:21Z

microcom.c


 struct ios_ops *ios;
 int debug;
+int quiet;


#55 explicitly zeros debug. Not zeroing quiet now aligns it with the code as-is, but since #55 should be merged first I think this should be: int quiet = 0;

Yes it's an artifact from my attempt to split up what was originally a single series. quiet is zero-initialized on the "single series" branch to which I can update this pr once the dependent PRs are merged.

I suppose unitialized globals are more a bad style practice than actual reliance on unitialized memory, at least on linux where they're zero-initialized as part of the bss (in contrast to locals which are stack-allocated and not automatically initialized).

Anyway, yes this will be taken care of :)

hnez · 2025-12-16T13:41:39Z

test/test_telnet.py

+                conn.sendall(buf)
+                time.sleep(0.01)  # sadly without this, microcom may miss the transmission
+                sock.close()


Could this hardcoded sleep result in flaky tests in the future?

Yes, definitely. I'm not sure why this is even needed. I'd expect that expect blocks until microcom has connected via TCP and that from this point whatever the server sends should definitely reach microcom. I suspect this is actually a workaround for a microcom bug.
As long as it's needed, should 0.01 turn out flaky on a slow runner, 0.1 should still run the tests quick enough.

But yes bad bad hack!

hnez · 2025-12-16T13:45:49Z

test/test_telnet.py

+@pytest.mark.parametrize("buf", [10, 1023, 1024, 1025, 4000])
+def test_no_cmd(telnet_recv, buf):
+    payload = make_pattern(buf)


Suggested change

@pytest.mark.parametrize("buf", [10, 1023, 1024, 1025, 4000])

def test_no_cmd(telnet_recv, buf):

payload = make_pattern(buf)

@pytest.mark.parametrize("length", [10, 1023, 1024, 1025, 4000])

def test_no_cmd(telnet_recv, length):

payload = make_pattern(length)

hnez · 2025-12-16T13:52:54Z

I like it! Since this adds python code to the repository: should the CI also run e.g. ruff format and ruff check now?

jluebbe · 2025-12-16T14:12:06Z

test/test_telnet.py

+@pytest.fixture
+def telnet_recv(cmd):
+    def _recv(buf, timeout=1):


Define helper functions directly; there is no need to use fixtures for that.

The reason that this is a fixture and not a helper function is that it directly depends on the cmd fixture. My understanding is that only tests and fixtures can depend on other fixtures, therefore a fixture is adequate here.

As there's little reason why a test that uses telnet_recv would also use cmd, there is no reason to pass the cmd fixture to the test only to then pass it on to telnet_recv.
So I'd prefer this:

def test_a(telnet_recv): assert telnet_recv("abc") == "abc"

Over this:

def test_a(cmd): assert telnet_recv(cmd, "abc") == "abc"

The difference is negligible in this case but adding more dependent fixtures to telnet_recv in the latter solution would be suboptimal, because they'd all need to be added to each of the tests even if the test code itself isn't touched

Using a fixture for helper functions such as these hides that telnet_recv actually uses the cmd fixture, making the code unnecessarily hard to read.

jonrebm · 2025-12-16T14:12:25Z

Thanks!
Well at least I could run ruff format next time I update this branch... But yes I'll add it to CI too if that's preinstalled on the runner atleast

microcom: add --quiet switch

53a3358

The quiet switch shall silence all printing to stdout during normal operation, but leaves the interactive console and all error messages functional. Useful for automation such as in CI. Signed-off-by: Jonas Rebmann <[email protected]>

jonrebm force-pushed the pytest branch from c50e0de to fad6758 Compare December 14, 2025 19:48

jonrebm added 5 commits December 14, 2025 20:57

CI: Make build workflow usable

d23c946

- correct branch to main, which our default branch - fix dependency installation - invoke pytest - don't build an unneeded release tarball Signed-off-by: Jonas Rebmann <[email protected]>

CI: build and test with ASAN and UBSAN

3b23acc

This has proven to be useful to catch out-of-bounds access and I assume will prevent many cases of success by chance when testing microcom. Signed-off-by: Jonas Rebmann <[email protected]>

CI: enforce style using uncrustify

8d31993

The rules are rather flexible so they should be enforcible treewide. If not, maybe there's a way to at least only enforce it on the sections or at least files that the commits touched? Signed-off-by: Jonas Rebmann <[email protected]>

CI: build and test with valgrind memory checker too

86dea61

Amongst others, this catches uninitialized memory access, which libasan does not. Signed-off-by: Jonas Rebmann <[email protected]>

jonrebm force-pushed the pytest branch from fbd0f44 to 86dea61 Compare December 14, 2025 19:57

hnez reviewed Dec 16, 2025

View reviewed changes

jonrebm marked this pull request as draft December 16, 2025 13:56

jluebbe reviewed Dec 16, 2025

View reviewed changes

Pytest tests and CI #58

Are you sure you want to change the base?

Pytest tests and CI #58

Uh oh!

Conversation

jonrebm commented Dec 14, 2025

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!

hnez commented Dec 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

jonrebm commented Dec 16, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

hnez commented Dec 16, 2025 •

edited

Loading