Global setting of test_engine and clone_db #89
Conversation
test/integration.jl
Outdated
| query = """ | ||
| def result { 1 } | ||
| """,) | ||
| @test_rel(name = "No input, expected, or output", query = """ |
There was a problem hiding this comment.
JuliaFormatter does not get on well with multi-line strings, which is most of @test_rel. Could you revert the formatting changes in this file?
| function set_test_engine!(new_engine::Option{AbstractString}) | ||
| return TEST_DEFAULTS[].engine = new_engine | ||
| end |
There was a problem hiding this comment.
This is a little redundant since there is already add_test_engine!(name::String) which adds the single listed engine to the pool. Given an empty pool the result is the same.
You are not the first to want to set a single default engine, though, so maybe it could map to
resize_test_engine_pool!(0)
add_test_engine!(name)
There was a problem hiding this comment.
I looked a bit more into the code, and I think there's still benefits in having this specific container for the "default engine". The solution I implemented here has 2 main advantages over mapping to resize_test_engine_pool! + add_test_engine!:
- It is way more efficient, because
resize_test_engine_pool!looks like a somewhat expensive operation, andadd_test_engine!needs to actually call the RAI API to try to create the engine, and then detect a409to know the engine already exists. This is very heavyweight for just setting the name of the default engine. - It does not mess up with the pool, so it can be used temporarily. That is, I can have a pool with a few engines, then I set the default engine to run a few tests, then when I unset the default engine and my pool is still there with my original engines. I understand this is not a great use case, but I think it's better if we don't have those side effects when setting the default engine name.
What do you think about this? I am fine either way, I can rewrite to delegate like you suggest, but in my opinion having an explicit default engine configuration would be better.
There was a problem hiding this comment.
My objection is just the (minor) added complexity, but given it's an implementation detail rather than API I'm fine with it either way.
| @@ -49,8 +114,10 @@ function create_test_database( | |||
| clone_db::Option{String}=nothing; | |||
| retries_remaining=3, | |||
| ) | |||
| db = isnothing(clone_db) ? get_default_clone_db() : clone_db | |||
| !isnothing(db) && @debug("Cloning '$name' from '$db'") | |||
| @@ -116,11 +116,11 @@ end | |||
| # Vectors. | |||
|
|
|||
| # :a => [3, 4, 5] | |||
| type_string(::Vector{T}) where T = type_string(T) | |||
| type_string(::Vector{T}) where {T} = type_string(T) | |||
There was a problem hiding this comment.
You have to thank JuliaFormatter!
| function set_test_engine!(new_engine::Option{AbstractString}) | ||
| return TEST_DEFAULTS[].engine = new_engine | ||
| end |
There was a problem hiding this comment.
My objection is just the (minor) added complexity, but given it's an implementation detail rather than API I'm fine with it either way.
This PR adds support to setting the
test_engineandclone_dbin a global fashion.The idea is that we can set which engine to use for running the tests, and from which database to clone for each test. We currently can do that on a per-
@test_relbasis, but we would like to support a way to globally define a default.This has been tested with the package manager testing harness.
This PR also includes changes done by the
JuliaFormatter.