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

Add support for Teardown, a run-always Finalizer #57

Closed
wants to merge 1 commit into from

Conversation

awelzel
Copy link
Contributor

@awelzel awelzel commented Aug 31, 2021

We're using btest to run system tests in a rather stateful environment
and were looking for a hook / entry point to run after every test for
cleanup and state checking.

Finalizer almost met the bill, except for not run for failing tests.

If running the executable given by Teardown fails, the whole test is
considered failed. If Teardown succeeds, but the test failed, the
test outcome is still failure.

We're using btest to run system tests in a rather stateful environment
and were looking for a hook / entry point to run after every test for
cleanup and state checking.

Finalizer almost met the bill, except for not run for failing tests.

If running the executable given by Teardown fails, the whole test is
considered failed. If Teardown succeeds, but the test failed, the
test outcome is still failure.
@awelzel awelzel changed the title [RFC] Add support for Teardown, a run-always Finalizer Add support for Teardown, a run-always Finalizer Aug 31, 2021
@awelzel
Copy link
Contributor Author

awelzel commented Aug 31, 2021

   [... File too long, truncated ...]
  == Diff ===============================
  --- /tmp/test-diff.16050._build.man.btest-sphinxdemo.1.baseline.tmp	2021-08-31 11:34:29.845074283 +0000
  +++ /tmp/test-diff.16050._build.man.btest-sphinxdemo.1.tmp	2021-08-31 11:34:29.841074211 +0000
  @@ -1,6 +1,6 @@
   .\" Man page generated from reStructuredText.
   .
  -.TH "BTEST-SPHINXDEMO" "1" "Aug 24, 2021" "1.0" "BTest-Sphinx Demo"
  +.TH "BTEST-SPHINXDEMO" "1" "Aug 31, 2021" "1.0" "BTest-Sphinx Demo"
   .SH NAME
   btest-sphinxdemo \- BTest-Sphinx Demo Documentation

Hmm - anything I should be doing about this? Seems @bbannier found that in #56 as well..

@bbannier
Copy link
Contributor

bbannier commented Aug 31, 2021

Hmm - anything I should be doing about this? Seems @bbannier found that in #56 as well..

This was found and fixed by @ckreibich in #56, and we are waiting for a merge for that.

@rsmmr rsmmr assigned rsmmr and unassigned rsmmr Aug 31, 2021
@rsmmr rsmmr self-requested a review August 31, 2021 12:30
Comment on lines +448 to +455
``Teardown``
An executable that will be executed after a test has run,
regardless of whether the test was successful or not.
It runs in the same directory as the test itself and receives
the name of the test and the number of failures as its parameters.
The return value indicates whether the test should indeed be
considered successful. By default, there's no teardown set.

Copy link
Member

Choose a reason for hiding this comment

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

Couple of questions about the semantics here:

  • Does the exit value get to override only on success, or also on failure (in other words: should a failing teardown script turn a successful test into a failure?), Also, do you indeed need the change-of-success for your use case? Might be easier semantics to just ignore the exit code so that teardown gets to clean up whatever it needs, but otherwise doesn't impact test execution.
  • What's the motivation for passing the number of failed tests so far? That seems ill-defined because one can't know when exactly the current test executes vs others, in particular with -j.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does the exit value get to override only on success, or also on failure (in other words: should a failing teardown script turn a successful test into a failure?)

This is currently implemented. A failing Teardown causes an otherwise successful test to fail.

Also, do you indeed need the change-of-success for your use case? Might be easier semantics to just ignore the exit code so that teardown gets to clean up whatever it needs, but otherwise doesn't impact test execution.

Adding @deejgregor to chime in here. One use-case was to fail a test if it leaves the environment in an unexpected state. The Teardown name might not fit the bill for this.

@bbannier pointed out that there might be better approaches here. I'll mark the MR as RFC.

What's the motivation for passing the number of failed tests so far? That seems ill-defined because one can't know when exactly the current test executes vs others, in particular with -j.

The description isn't great. It's the number of cmd failures (failures variable) a given test had. No actual use-case currently, but could allow the teardown script to take conditional actions depending whether the test had succeeded or failed.

Copy link
Contributor

@bbannier bbannier Aug 31, 2021

Choose a reason for hiding this comment

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

Capturing the points from an offline discussion I had with @awelzel:

  • I believe that if you want to validate that a test didn't leave broken external state you should explicitly test for that. Having this in the BTest test case would be most explicit, but it should also be possible to use a Finalizer for that. This wouldn't capture unexpected external state from failing tests, but if a test fails you already know that something is broken.
  • If you want to validate that each test starts with proper external state you could use an explicit test step or Initializer to validate or reset the external state. Without knowing too much about the external state you are dealing with, resetting the state before a test seems superior as it would allow other tests to run after a prior test has broken the state.

An additional Teardown step introduces more places where e.g., a test's data or state could get mutated (in addition to the existing Finalizer), and I still don't see a clear cut use case for Teardown which would outweigh the complexity it would introduce. I already am slightly biased against Initializer/Finalizer since they seem to move test code (they can fail a test) out of the actual test case making it harder to follow what is happening; they also seem overly broad for something which can decide a test's status, but would be applied to all test cases. Teardown would add more of such functionality.

@awelzel awelzel marked this pull request as draft August 31, 2021 13:03
@ckreibich
Copy link
Member

Fwiw, I just hit a scenario where I'd quite like an unconditional setup and teardown. I'm running btests on a docker-compose setup, and for those the compose setup is just framing — I need it to happen before each test and after, no matter what the test itself does. I'm working around it with Bash EXIT traps for now.

@bbannier
Copy link
Contributor

Since it is unclear whether we actually want to solve the problem here let's close this for now.

@bbannier bbannier closed this Sep 13, 2021
@ckreibich ckreibich mentioned this pull request Oct 19, 2021
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.

None yet

4 participants