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

Tests run_until should trigger automatic cleanup #617

Open
ArneTR opened this issue Dec 22, 2023 · 2 comments
Open

Tests run_until should trigger automatic cleanup #617

ArneTR opened this issue Dec 22, 2023 · 2 comments
Assignees

Comments

@ArneTR
Copy link
Member

ArneTR commented Dec 22, 2023

Hey @dan-mm

we had a nasty issue that the tests were not running correctly because the run_until function does not automatic cleanup.

My question is: Why?

Is this behaviour somewhere needed? IT seems very risky to have no automatic cleanup as the metric providers not being properly closed will cascade into other tests and makes this very hard to find.

@davidkopp Tagged you fyi. If you write a new test please use the cleanup guard clause for the moment as done in PR #616 . As you can see we are discussing internally if we fix that test suite design, as it is very unintuitive that a specifically written test case can destroy other test cases

@dan-mm
Copy link
Contributor

dan-mm commented Jan 9, 2024

There is a specific reason for this, though I am very much open to feedback on better ways to do it as I agree that it is easy to break.

The point of the Tests.run_until funciton is that we want to instrument the GMT up until a certain point, then effectively pause it, and run some checks. Sometimes these checks need the containers to still be up and available for inspection, so cleanup cannot be run yet.

In order to be able to do this, I have split the instrumentation into two functions in TestFunctions - run_until and cleanup. You are not meant to use run_until without calling cleanup afterwards as well (I realize now this should be documented).

here's an example:

def get_env_vars(runner):
    try:
        Tests.run_until(runner, 'setup_services')

        ps = subprocess.run(
            ['docker', 'exec', 'test-container', '/bin/sh',
            '-c', 'env'],
            check=True,
            stderr=subprocess.PIPE,
            stdout=subprocess.PIPE,
            encoding='UTF-8'
        )
        env_var_output = ps.stdout
    finally:
        Tests.cleanup(runner)
    return env_var_output

Because the implementation of what checks you want to make depends on the tests, I don't see a way to automatically trigger this cleanup.

I agree this is fragile and quite easy to break, especially if the Tests.cleanup functionality gets changed from the runner.cleanup functionality (the same is true for the run_until functionality though). I'm open to suggestions.

@dan-mm
Copy link
Contributor

dan-mm commented Jan 9, 2024

One potential way to do this is with a destructor:

Look into using del - make the test-runner a class, with this del destructor that will call the cleanup phase

Also worth thinking about having a test-runner class as a child of the runner class to inhereit the cleanup, so we do not have to mirror it.

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

No branches or pull requests

2 participants