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

test: follow-up on the integration tests #353

Merged
merged 7 commits into from
Apr 11, 2024
Merged

test: follow-up on the integration tests #353

merged 7 commits into from
Apr 11, 2024

Conversation

dvdhrm
Copy link
Member

@dvdhrm dvdhrm commented Apr 11, 2024

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

  • I dropped the debug-statements that print the selected binary in each test-run. If we want this, lets introduce this properly and print it once per test, rather than on each use.
  • Change install-tests to tests in meson_options.txt. See the commit-message for the rationale.

dvdhrm added 7 commits April 11, 2024 12:09
Use proper curly brackets around all blocks, and ensure we use
`c_assert()` over `assert()`.

Signed-off-by: David Rheinsberg <[email protected]>
Do not print the executed binary as test-output. We have no such debug
output in our other tests. If we ultimately decide to include it, we
should fetch this information early and pass it to each test, rather
than print it over and over.

Signed-off-by: David Rheinsberg <[email protected]>
Change the configuration option to include tests in the distribution to
just `tests`. We try to never conditionalize which parts to build, but
rather always build everything that can be built with the selected
dependencies. Hence, a `tests` configuration would never imply that it
builds tests, and we can safely use it to mean the tests will be part of
the distribution.

There are some exceptions to this rule, but those we likely cannot
change for compatibility reasons.

Signed-off-by: David Rheinsberg <[email protected]>
Use the kwargs to share all common test arguments and then reduce the
verbosity by keeping all tests on a single line each.

Signed-off-by: David Rheinsberg <[email protected]>
Do not test for aux-groups in credential reports on dbus-daemon. This is
a relatively new feature and might not be reported by the tested
instance.

Signed-off-by: David Rheinsberg <[email protected]>
Use proper suite-names for the different tests, rather than using the
man-page references from our documentation.

Signed-off-by: David Rheinsberg <[email protected]>
Use proper path-joining operators of meson, rather than
string-concatenation.

Signed-off-by: David Rheinsberg <[email protected]>
@dvdhrm
Copy link
Member Author

dvdhrm commented Apr 11, 2024

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!

@dvdhrm dvdhrm merged commit c75158d into bus1:main Apr 11, 2024
37 checks passed
@mrc0mmand
Copy link
Contributor

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
Copy link
Member Author

dvdhrm commented Apr 11, 2024

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!

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