1.0 release and do not automatically create if missing#130
1.0 release and do not automatically create if missing#130
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.
| `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. |
omus
left a comment
omus
left a comment
There was a problem hiding this comment.
Very late to the party. Overall looks reasonable but there are a few issues to sort out.
| @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.
| withenv("JULIA_REFERENCETESTS_UPDATE"=>true) do | |
| withenv("JULIA_REFERENCETESTS_UPDATE" => "true") do |
| @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.
| withenv("JULIA_REFERENCETESTS_UPDATE"=>true) do | |
| withenv("JULIA_REFERENCETESTS_UPDATE" => "true") do |
| 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.
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.
| 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.
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
3 participants
@omus can you review?
This should solve #127