Conversation
sophokles73
force-pushed
the
run_tests_in_ci_build
branch
7 times, most recently
from
22b2a0e to
269ee54
Compare
The doc test for the async client wait() method assumed an MQTT broker to be running. This commit updates the test to assert that an error is returned when waiting on the connection being established, which should usually be the case.
More recent versions of the toml_writer crate, which is used for development, have an MSRV of 1.76. A toolchain for the crate's indicated MSRV of 1.75 therefore fails to build the crate. The MSRV has been slightly raised to 1.76 to remedy the issue and the version of the async-lock crate has been pinned to 3.4.1, which is the last version to support Rust 1.76.
Added a job to the GitHub Actions workflow to run the tests as part of the CI build. This ensures that any changes that break the tests will be caught early in the development process.
sophokles73
force-pushed
the
run_tests_in_ci_build
branch
from
269ee54 to
90d1af2
Compare
|
Most of this looks good. The one potential problem is the bump to Rust v1.76. The last time I jumped the MSRV, I got a number of complaints from people using Yocto Linux LTS, which is currently Scarthgap with Rust v1.75 So I wanted to target v1.75 for this release so as not to break their builds. This creates the problem of dependencies breaking us. For users, I will leave them to manage their own Cargo.lock files, so long as we know we can resolve to compatible versions. That just leaves the CI. One thing I did recently in another project was to use the latest stable in the CI to use the new resolver to generate the lock file, then build with the MSRV using that lock file. It's a bit painful, but better than breaking the build for users. Hopefully, this will all go away in a few months when Yocto jumps to it's next major version, hopfully bringing us beyond v1.85. This is also better than trying to pin dependencies. That is usually only a temporary solution as I've repeatedly seen a dependency of the pinned dependency jump over the MSRV. |
I see
And the thing about the new resolver is its ability to consider the project's MSRV, I guess?
Agreed |
|
I fixed the CI to resolve the dependencies before checking the build and clippy for the MSRV. Doing that I was able to keep the MSRV at 1.75. Then I tried out running the unit tests with a slightly modified version of what you have here, again resolving the dependencies for the MSRV, and that all worked. So now it won't merge cleanly and there's a few things we don't want in here (MSRV 1.76, modified doc test). But the rest should go in (the single setup for the CI & some code cleanup). Would you want to fix and rebase this? If not, I can pick through it and get the rest in. Either way, thanks for the help! |
By all means, go ahead and pick through it. I will close this PR then and concentrate on my PR for fixing #244. BTW which branch should I target it to, develop or master or something else? |
|
I try to keep master stable. Most new development is done against develop, so that's the best branch to target a PR. |
2 participants
Currently, running
cargo test --docwill fail if no MQTT broker is running on
localhost:1883. This is due to a doc test inasync_client.rsthat assumes such a broker to be running.This PR adapts the doc test to not expect connecting to the broker actually succeeds.
Also, the PR adds a job to the GitHub Actions workflow to run the tests as part of the CI build. This ensures that any changes that break the tests will be caught early in the development process.