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

Isolated temp dir per run #283

Closed
dimo414 opened this issue Apr 25, 2020 · 16 comments
Closed

Isolated temp dir per run #283

dimo414 opened this issue Apr 25, 2020 · 16 comments
Milestone

Comments

@dimo414
Copy link
Contributor

@dimo414 dimo414 commented Apr 25, 2020

The README points to BATS_TMPDIR as a "location to a directory that may be used to store temporary files", but currently it's (typically) just /tmp or $TMPDIR, meaning that anything written to that directory will affect subsequent runs.

BATS itself appears to instead use BATS_TMPNAME which is keyed off the PID (which still isn't safe, as the OS will reuse PIDs over time), but it's not clear whether users are supposed to use BATS_TMPNAME or if it's an implementation detail.

I would suggest providing a different variable, e.g. BATS_TEST_TMP, that is unique per-run, and documenting that in the README instead of BATS_TMPDIR. I'd also suggest changing BATS_TMPNAME to not (only) rely on the PID, but I'm not sure if there are additional constraints with that - BATS_PARENT_TMPNAME makes me suspicious parts of BATS currently relies on this naming scheme.

@rico-chet
Copy link
Contributor

@rico-chet rico-chet commented Apr 25, 2020

The problem of handling temporary directories is solved, the question is whether it belongs to the core and the convenient answer is no. Which percentage of tests need this functionality? We probably don't know, and a smaller focused library is better than a bigger broader one.

So, I rather support #276.

@dimo414
Copy link
Contributor Author

@dimo414 dimo414 commented Apr 25, 2020

"solved" is a bit strong given that this requires a separate library that hasn't even been pulled into the bats-core org yet.

It's reasonable to decide that file-system dependent assertions and test utilities should live in a separate library, but bats-core is responsible for test setup and teardown, and for the environment made available to tests. The existing documented behavior is error-prone, and will remain so even if people are supposed to use bats-file. I'd be happy to see that issue resolved, but it doesn't address this problem.

@rico-chet
Copy link
Contributor

@rico-chet rico-chet commented Apr 25, 2020

Bats is a TAP-compliant testing framework for Bash.

So bats-core is concerned about TAP-compliance and the framework side of things, batteries, canvas, paint and brushes not included.

We could debate endlessly. ;-)

@dimo414
Copy link
Contributor Author

@dimo414 dimo414 commented Apr 25, 2020

Maybe we're talking past each other - are you suggesting the current behavior of BATS_TMPDIR (and BATS_TMPNAME) is a feature and not a bug?

If you don't think any file system hooks should be provided by bats-core that's fair, but in that case BATS_TMPDIR should be removed (or at least not documented).

@waterkip
Copy link
Contributor

@waterkip waterkip commented Apr 28, 2020

mktemp -d would be helpful instead of using the pid.

mkdir -p $TMPDIR/bats
BATS_TMPDIR=$(mktemp -d $TMPDIR/bats.XXXXXX)

Maybe add some trap to clean it up after bats terminates, from either a regular return or all other signals.

I would not advise users to use that for their tests, just let them run mktemp -d themselves. They can then decide on their own if they want to remove it after the test or they just don't care. You can help them with providing a helper function, but the command is rather trivial to invoke yourself, so... why bother?

@rico-chet
Copy link
Contributor

@rico-chet rico-chet commented Apr 28, 2020

Since bats-file moved to bats-core recently, which implements tmpdir helpers nicely, why bother? If one needs a tmpdir, then one of also likely to need some file-related assertions, so that is a good fit, IMHO.

Having BATS_TMPDIR available to users is rather pointless though, it's easily mistaken to be disposable.

@dimo414
Copy link
Contributor Author

@dimo414 dimo414 commented Apr 28, 2020

Anecdotally, I've needed isolated temporary directories without needing custom file assertions. For example, testing a program that caches data somewhere requires having a isolated "somewhere" to point it at, even if you don't actually need to inspect the resulting contents of the cache.

@rico-chet
Copy link
Contributor

@rico-chet rico-chet commented May 10, 2020

Well, you could have asserted that the cache directory is actually used ;-)

I can imagine bats-core providing special isolation modes like:

  • test-specific or test suite-specific home directory (by overriding $HOME) - instant
  • test-specific or test suite-specific rootfs (by means of containers or jails) - slow

Not sure how to accomodate for this syntactically, as there are no test attributes (yet).

@dimo414
Copy link
Contributor Author

@dimo414 dimo414 commented May 10, 2020

I really don't see why the existing behavior that BATS runs everything in the same directory (especially across runs!) would be desirable. Is there some negative consequence of changing this behavior that you're concerned about?

Put another way, suppose BATS had always run tests in isolated directories; would you want to see that replaced with the current behavior?

@rico-chet
Copy link
Contributor

@rico-chet rico-chet commented May 11, 2020

While I'm not a bats-core developer, I would guess that's for simplicity reasons. Simplest thing that could possibly work is to have a single directory for all tests.

@dimo414
Copy link
Contributor Author

@dimo414 dimo414 commented May 12, 2020

But it doesn't work, that's the motivation for this issue :) Running separate tests in the same directory, and especially sharing that directory across different runs, is a recipe for unexpected behavior. Any state a test changes will persist for the next test or next run, which can introduce flakiness, brittleness, or hard-to-reproduce issues. The test framework should not force this sort of non-hermetic behavior on tests.

@rico-chet
Copy link
Contributor

@rico-chet rico-chet commented May 15, 2020

You still silently assume that a test needs a directory to run, just because your tests do.

My point is that with the temporary directory isolation alone, tests' behavior will remain non-hermetic. Tools access home directory, /etc, /usr/share, /var/lock, you name it. When I want to take unexpected behavior, flakiness, brittleness, or hard-to-reproduce issues out, I run tests in a rootfs-isolated environment (chroot/container/jail) or even run each test in a separate one. I don't find bats-core limiting me in any regard when I do this. And bats-core doesn't promise any isolation, so I don't expect any. If it promised a bit of isolation here and there, I'd become rather confused to where the limits lie.

@dimo414
Copy link
Contributor Author

@dimo414 dimo414 commented May 17, 2020

I agree with your premise, but BATS is already in an inconsistent state; the documentation recommends using BATS_TMPDIR in tests. One way to resolve this issue would be to simply stop documenting these problematic variables.

Personally, I think it would be better to actually give each test (or at least each bats invocation) a separate directory, but it's fine to say test authors have to figure out what they need for themselves. Either way, BATS_TMPDIR as-implemented is problematic.

@rico-chet
Copy link
Contributor

@rico-chet rico-chet commented May 19, 2020

I agree that BATS_TMPDIR as-implemented is confusing and of little value.

dimo414 added a commit to dimo414/bash-cache that referenced this issue Jun 13, 2020
@jbergstroem
Copy link

@jbergstroem jbergstroem commented Jul 8, 2020

I would love to see this too. Had to create a wrapper in order to handle the creation (and destruction) of said "test run" folder. I'm not a big fan though since I now longer can invoke bats directly, meaning passing --filter or -j becomes tedious.

This as well as having a global (per test run) setup/teardown would be great additions to managing state.

@martin-schulze-vireso
Copy link
Member

@martin-schulze-vireso martin-schulze-vireso commented Oct 9, 2020

There is a partial solution since v1.2.1. See this comment: #226 (comment)

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

5 participants