-
Notifications
You must be signed in to change notification settings - Fork 15
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
1.0 release and do not automatically create if missing #130
base: master
Are you sure you want to change the base?
Conversation
We can't do the thing anymore in CI where we delete the files and rerun the tests twice to see if they regenerate correctly. Possibly should normalize the Unit test file somewhat since not doing that anymore? |
@omus can I have a review? |
results don't match. This dialog allows the user to update the | ||
reference files. If you do not want to be prompted, just | ||
delete the reference data before running the tests. | ||
`include`), then it will trigger an interactive dialog if the refrence is missing or the results don't match. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
`include`), then it will trigger an interactive dialog if the refrence is missing or the results don't match. | |
`include`), then it will trigger an interactive dialog if the reference is missing or the results don't match. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very late to the party. Overall looks reasonable but there are a few issues to sort out.
@@ -162,21 +162,25 @@ end | |||
mktempdir() do path | |||
newfilename = joinpath(path, "newfilename.$ext") | |||
@assert !isfile(newfilename) | |||
with_logger(test_logger) do | |||
@test_reference newfilename val # this should create it | |||
withenv("JULIA_REFERENCETESTS_UPDATE"=>true) do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
withenv("JULIA_REFERENCETESTS_UPDATE"=>true) do | |
withenv("JULIA_REFERENCETESTS_UPDATE" => "true") do |
|
||
@testset "Create new image as txt" begin | ||
# This is a separate testset as need to use the `size` argument to ``@test_reference` | ||
mktempdir() do path | ||
newfilename = joinpath(path, "new_camera.txt") | ||
@assert !isfile(newfilename) | ||
with_logger(test_logger) do | ||
@test_reference newfilename camera size=(5,10) # this should create it | ||
withenv("JULIA_REFERENCETESTS_UPDATE"=>true) do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
withenv("JULIA_REFERENCETESTS_UPDATE"=>true) do | |
withenv("JULIA_REFERENCETESTS_UPDATE" => "true") do |
end | ||
end | ||
|
||
force_update() = tryparse(Bool, get(ENV, "JULIA_REFERENCETESTS_UPDATE", "false")) === true | ||
force_update() = tryparse(Bool, get(ENV, "JULIA_REFERENCETESTS_UPDATE", "false")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems problematic as this function can now return nothing
which will fail when it's used in a conditional. I do think it's reasonable to throw an exception if a user sets this incorrectly though:
force_update() = tryparse(Bool, get(ENV, "JULIA_REFERENCETESTS_UPDATE", "false")) | |
force_update() = parse(Bool, get(ENV, "JULIA_REFERENCETESTS_UPDATE", "false")) |
Alternatively, we can just silently default to false
:
force_update() = tryparse(Bool, get(ENV, "JULIA_REFERENCETESTS_UPDATE", "false")) | |
force_update() = get(ENV, "JULIA_REFERENCETESTS_UPDATE", "false") == "true" |
function handle_mismatch(reference_file, actual_path) | ||
if !isinteractive() && !force_update() | ||
error(""" | ||
To update the reference images either run the tests interactively with 'include(\"test/runtests.jl\")', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To update the reference images either run the tests interactively with 'include(\"test/runtests.jl\")', | |
To update the reference images either run the tests interactively with `include(\"test/runtests.jl\")`, |
rm -rf test/references | ||
julia --color=yes --check-bounds=yes --project -e "using Pkg; Pkg.test(coverage=true)" | ||
julia --color=yes --check-bounds=yes --project -e "using Pkg; Pkg.test(coverage=true)" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like a bad idea to remove the tests which ensure the test references can be created from scratch. I would make this a separate step in the workflow though as why we do this isn't clear currently
@omus can you review?
This should solve #127