Skip to content

Fix doc test for async_client#262

Closed
sophokles73 wants to merge 1 commit intoeclipse-paho:masterfrom
sophokles73:fix_doc_test
Closed

Fix doc test for async_client#262
sophokles73 wants to merge 1 commit intoeclipse-paho:masterfrom
sophokles73:fix_doc_test

Conversation

@sophokles73
Copy link
Contributor

The doc test for the async_client::wait function 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.

The doc test for the async_client::wait function 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.
@sophokles73
Copy link
Contributor Author

this fix is also included in #260
maybe @fpagliughi can provide some guidance as to how you want to proceed with this?

@fpagliughi
Copy link
Contributor

I'm preparing a release to go out next week, so a bunch of things will break and be fixed in the next few days.

For the CI, my hope was to spin up a version of Mosquitto in the Actions containers to test against.

@sophokles73
Copy link
Contributor Author

I'm preparing a release to go out next week, so a bunch of things will break and be fixed in the next few days.

Good to know. I do have a fix for #244 that I would really like to get into that release, if you don't mind.

For the CI, my hope was to spin up a version of Mosquitto in the Actions containers to test against.

That's fine for running the tests on CI, but when running in your local workspace, it feels odd that executing the tests fails ...

@fpagliughi
Copy link
Contributor

That's fine for running the tests on CI, but when running in your local workspace, it feels odd that executing the tests fails ...

Yeah, sorry. You don't know what is in the branches I haven't pushed yet! :)

For just this one doc test we can ignore the error.

I started another branch (not pushed yet) to to implement and test some improved error handling. For that I want to have unit regression tests to make sure the error results stay consistent. But I don't plan on setting up mocks to do it; so I need to test against a broker.

What I can do (and have done on some other crates) is to have a cargo build feature, like "broker-tests", which would require a compliant broker on localhost to pass. That way these tests would be opt-in, so shouldn't surprise anyone.

And then set up the CI to run the broker in the container.

@fpagliughi
Copy link
Contributor

Oh, just to clarify, I will close this and set the doc comment to ignore.

I would rather not have code in the source comments that looks like it shouldn't pass. I worry that might be confusing.

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