bus1 / dbus-broker

Linux D-Bus Message Broker
https://github.com/bus1/dbus-broker/wiki
Apache License 2.0
661 stars 78 forks source link

test: follow-up on the integration tests #353

Closed dvdhrm closed 3 months ago

dvdhrm commented 3 months ago

A collection of follow-ups on the recent integration test additions by @mrc0mmand. I think the major two changes are:

dvdhrm commented 3 months ago

I will merge this now, since I have to prepare a bug-fix release, and I would like the configuration change in. But I will happily change things later on, even if this is merged, so please comment if anything needs further discussion!

mrc0mmand commented 3 months ago

I will merge this now, since I have to prepare a bug-fix release, and I would like the configuration change in. But I will happily change things later on, even if this is merged, so please comment if anything needs further discussion!

Looks good to me, thank you!

Maybe one small comment about d630cd1 - printing this out multiple times per test is indeed annoying but having some confirmation that we run the tests against an expected binary would be nice. But that's definitely a minor detail that can be figured out later.

dvdhrm commented 3 months ago

Yeah, I can understand that introspection is nice to have. I just thought we should move that to a separate PR, and I didn't want to stall your PR further.

We generally do not output anything from the tests so far. On the contrary, we expect STDOUT/STDERR to be under full test control (e.g., the error tests are certainly allowed to assert on traffic on stderr/stdout). Hence, I kinda dislike using it for user-targetted output. Then again, maybe the error tests should be special cased.

Anyway, if there is interest, lets suggest this separately.

Thanks a lot for the feedback!