Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Clean up integration tests #181

Merged
merged 6 commits into from
May 5, 2024
Merged

Conversation

sirhcel
Copy link
Contributor

@sirhcel sirhcel commented May 5, 2024

This PRs cleans up some warnings from the doctests and makes tests using serial interfaces putting their cards on the table: The results of the operations under tests were just ignored beforehand and will be checked now.

Having the results checked now, requires the tests to use serial devices designated for testing. To keep a low profile, this is done by passing the devices via environment variables and rests requiring actual serial hardware can be ignored by default with the feature ignore-hardware-tests which is done in CI.

sirhcel added 6 commits May 4, 2024 21:49
Explicitly use the variables previously reported as unused. Also
harmonize mutability of pseudo terminal pair.
Previously, tests were reported successful in every case. For example
even with non-existent devices. So make the serial ports to use for
testing explicit and actually check for success.
We have no actual hardware to test with in CI. This way hardware tests
are at least built and the test code checked. Ignoring these tests
clearly marks them during test execution as well.

Passing the serial devices to use for testing via environment variables
keeps a low profile. This is also a footgun when changing these devices.
Improvements like conveniently using a config file are welcome.

I found no straight forward solution for sharing the environment
variables between different CI workflows and so they are duplicated with
a note for now.
This surfaced with 1.80.0-nightly (05364cb2f 2024-05-03). According to a
search in the targets mentioned in
https://github.com/rust-lang/reference it always has been 'dragonfly'
and just the warning is new.
We are running CI builds and tests for several of them, but not all.
@sirhcel sirhcel requested a review from eldruin May 5, 2024 20:53
Copy link
Contributor

@eldruin eldruin left a comment

Choose a reason for hiding this comment

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

Nice work, thank you!

@eldruin eldruin merged commit 392fc02 into serialport:main May 5, 2024
30 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants