Proposal: ExUnit.on_exit callback with test state

[Christopher Keele]

I am creating a resource during test setup based on a test's context (namely, its module and name):

setup context do
  prepare_resource_for_test_context(context)
end

I would like to be able to teardown this resource upon test conclusion. This is possible today:

setup context do
  prepare_resource_for_test_context(context)

  on_exit do

    cleanup_resource_for_test_context(context)

  end

end

Proposal

However, I would also like to leave the resource in-place on test failure for inspection. (My prepare_resource function can handle the situation where a resource already exists on setup. This is opposite to the common tmpdir pattern where the resource cleans itself up automatically upon test conclusion but would have unexpected side effects if already setup.)

As far as I know, this is not possible today. I would like to do something like receiving the ExUnit.state() in the on_exit callback:

setup context do
  prepare_resource_for_test_context(context)

  on_exit state do

    case state do

      {:failed, _} -> :ok

      _ -> cleanup_resource_for_test_context(context)

    end

  end

end

Are there reasons to not entertain this functionality? Is there another way to accomplish it that doesn't rely on a test formatter to notice {:test_finished, test} and do the cleanup at a global level?

Implementation

AFAICT we could enable this usecase trivially by threading test_or_case.state into ExUnit.OnExitHandler.run and on to its helper functions.

We could retain backwards-compatibility by having exec_callback(callback, state) check the arity of the callback before invoking it.

Documentation could describe the optional callback parameter and elaborate that an on_exit callback defined in a setup_all would have some other behaviour (raise an error, receive nil or the case name instead of a state, etc—open to ideas).

Are would such an implementation be welcome?

[Christopher Keele]

Assuming, of course I wrote this proposal without the typo in the final sentence and properly used an anonymous function for on_exit(fn state -> #... end) in my examples.

[José Valim]

> However, I would also like to leave the resource in-place on test failure for inspection. (My prepare_resource function can handle the situation where a resource already exists on setup. This is opposite to the common tmpdir pattern where the resource cleans itself up automatically upon test conclusion but would have unexpected side effects if already setup.)

What we typically do is that we never delete it by default, instead we clean up when the next test runs. The rationale is:

1. Leave resources for debugging

2. If tests are interrupted (ctrl+c or whatever other reason), you need to deal with trailing resources anyway

Would that be a problem here?

José Valim

https://dashbit.co/

--
You received this message because you are subscribed to the Google Groups "elixir-lang-core" group.
To unsubscribe from this group and stop receiving emails from it, send an email to elixir-lang-co...@googlegroups.com.
To view this discussion visit https://groups.google.com/d/msgid/elixir-lang-core/71b223e9-3815-4b8d-9ac0-cb32b98b05e8n%40googlegroups.com.

[Christopher Keele]

> What we typically do is that we never delete it by default, instead we clean up when the next test runs. The rationale is:

> 

> 1. Leave resources for debugging

> 2. If tests are interrupted (ctrl+c or whatever other reason), you need to deal with trailing resources anyway

> 

> Would that be a problem here?

What I'm toying with is a fork of Ecto.Adapters.SQL.Sandbox that implements test isolation by doing a full clean copy of the test database during setup per test, each copy named after the test in question, and redirecting all test interaction to the copy, rather than using transactions to get isolation. The goal is to support concurrent tests for Sqlite and to allow introspecting database state on test failure for Sqlite and Postgres. So in this case,

1. Tearing down the resource unconditionally on_exit is not desired (to have a debuggable state left)

2. Leaving behind every resource on successful tests is not desirable (that's a lot of wasted disk space after running 2000 stateful tests)

3. Keeping resources for failed tests and juggling N extra copies on disk where test parallelism is N is just about manageable

Under this mechanism, leaving test failure resources behind is manageable, as is leaving N copies behind on test interruption, and all leftover state is overwritable on next test run, but leaving all copies behind is not viable for non-trivial test suites sizes or large seed databases. The worst case then becomes some errant global configuration causing every stateful test to fail—the mostly likely way that would happen is by providing a bad database configuration itself, which would prevent creation in the first place, preventing disk bloat from happening to begin with.

[José Valim]

Please do give a pull request a try then!

José Valim

https://dashbit.co/

To view this discussion visit https://groups.google.com/d/msgid/elixir-lang-core/b4f28831-a68d-4d27-8239-7042cc64c469n%40googlegroups.com.

[Ben Wilson]

Ah yes we've run into this exact thing doing tests in Clickhouse. Even for postgres, this would be amazing for testing features like pg_notify or other postgres features that depend on an actual COMMIT.

[Christopher Keele]

> > What I'm toying with is a fork of Ecto.Adapters.SQL.Sandbox that implements test isolation by doing a full clean copy of the test database during setup per test

> Ah yes we've run into this exact thing doing tests in Clickhouse. Even for postgres, this would be amazing for testing features like pg_notify or other postgres features that depend on an actual COMMIT.

Ben, if you are interested, what I am toying with is ultimately bringing pytest-postgresql to all of Ecto: a clone-based transaction-free sandbox mechanism for any db_connection-powered Ecto adapter (not just for Ecto.SQL):

• Adding a new optional Ecto.Storage.storage_clone type-callback
  • Implementing this for adapters where the storage has underlying conveniences for this, ex
    • easy to implement (built-in):
      • postgres: CREATE DATABASE TEMPLATE test_db
      • sqlite3: File.copy("test_db")
    • cheap to implement (copy on write semantics)
      • snowflake: CREATE DATABASE CLONE test_db
      • clickhouse: CREATE DATABASE test_db and loop CLONE TABLE
    • possibly high-value but laborious to get right adapters:
      • MyXQL, TDS, etc
• Rewriting the Ecto.Adapters.SQL.Sandbox to work for any ecto adapter powered by db_connection, not just from Ecto.SQL
  • since storage_clone/storage_down is sufficient for any compatible adapter to guarantee isolation, we could offer sandboxing to any compatible ecto storage
• Allowing connections to specify the isolation strategy as :transactional vs :clone
  • falling back preferentially to :transactional and the existing sandbox mechanism for supported Ecto.SQL adapters, :clone for everything else if storage_clone is supported

I don't know what the final form of this experiment will be, but when I get some anticipated downtime in a couple of weeks to work on it I'd love to pick your brains if you have immediate use-cases! This proposal is just setting the stage for that effort. José has brought up a viable alternative for libraries in the PR, so it's non-blocking, but I'd still like to get a blessed API for this kind of cleanup into ExUnit if I can anyways as it seems useful and less brittle than hooking into the ExUnit process lifecycle.

[Ben Wilson]

Very exciting. What I think is interesting is that in particular for the databases where creating the clone is easy but not "instantaneous" (eg clickhouse) you'd almost want to have a pool of pre-allocated sandboxes and then when a test needs a sandbox it grabs one and while the test is doing its thing the sandbox manager could be allocating another sandbox in the background. If you had a lot of tables to clone (and assuming clickhouse can do DDL changes in parallel for different DBs, I think it can?) you could probably stay ahead of the tests and not pay the sandbox initialization price at the front of every case. Might be a premature optimization though.