reduce test stage dependency with ReferenceTests #14
Conversation
|
Thank you Jonny for suggesting ReferenceTests.jl again. I'll check the PR
tomorrow morning but I agree that visual regression tested is too complex
and unreliable.
…On Thu, Jul 2, 2020, 01:22 Johnny Chen ***@***.***> wrote:
As @mgautam98 <https://github.com/mgautam98> is going to improve this
package with AbstractArray{<:Colorant} support ( #13
<#13> ) , I'm
doing some preparation work now. This one is for the test, and I'll open a
PR for CI later.
I'm still resistant to the usage of VisualRegressionTests for two reasons:
- Even if it's a test-only dependency, VisualRegressionTests is still
very large a dependency with Plots and Gtk.
- The reference file can be very fragile because the output depends on
how backends (Gtk) draw the figure. The failure in
https://travis-ci.org/github/JuliaImages/ImageInpainting.jl/builds/647806320
is one such case. On the other hand, ReferenceTests only depends on image
IO backend, which is quite faithful and reliable.
@juliohm <https://github.com/juliohm> Are you okay with this change?
------------------------------
You can view, comment on, or merge this pull request online at:
#14
Commit Summary
- reduce test stage dependency with ReferenceTests
File Changes
- *M* Project.toml
<https://github.com/JuliaImages/ImageInpainting.jl/pull/14/files#diff-910a7b3ca0075d545dcec45cb7335ca9>
(19)
- *M* test/criminisi.jl
<https://github.com/JuliaImages/ImageInpainting.jl/pull/14/files#diff-9da901d4e1c971ae6b42ee56bd46c661>
(16)
- *D* test/data/Blobs.png
<https://github.com/JuliaImages/ImageInpainting.jl/pull/14/files#diff-0fdb16a597b3c228d969da74f6a4c204>
(0)
- *D* test/data/Cross.png
<https://github.com/JuliaImages/ImageInpainting.jl/pull/14/files#diff-9d3689ab17772c47fbe1bfff3a77ce96>
(0)
- *D* test/data/Diagonal.png
<https://github.com/JuliaImages/ImageInpainting.jl/pull/14/files#diff-04224ecbb684cfc1ee271f32fcbebd0a>
(0)
- *D* test/data/LightHouse.png
<https://github.com/JuliaImages/ImageInpainting.jl/pull/14/files#diff-8b596d9cdd8d529569fddbca5f1add4b>
(0)
- *A* test/data/criminisi/Blobs.png
<https://github.com/JuliaImages/ImageInpainting.jl/pull/14/files#diff-49b7e35074662a0eceff32721f6b2498>
(0)
- *A* test/data/criminisi/Cross.png
<https://github.com/JuliaImages/ImageInpainting.jl/pull/14/files#diff-37e5705bc346d083dbebeb5d5f9620d5>
(0)
- *A* test/data/criminisi/Diagonal.png
<https://github.com/JuliaImages/ImageInpainting.jl/pull/14/files#diff-54853acb7bafb021615781a9d71b659c>
(0)
- *A* test/data/criminisi/LightHouse.png
<https://github.com/JuliaImages/ImageInpainting.jl/pull/14/files#diff-dac47da390746b06151c83c93bf4c6d5>
(0)
- *M* test/runtests.jl
<https://github.com/JuliaImages/ImageInpainting.jl/pull/14/files#diff-fce720c43af3c52c862fd7451c7374b8>
(28)
Patch Links:
- https://github.com/JuliaImages/ImageInpainting.jl/pull/14.patch
- https://github.com/JuliaImages/ImageInpainting.jl/pull/14.diff
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#14>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAZQW3KA5JBNRVKCSMNNBJLRZQDPRANCNFSM4OOP3VQQ>
.
|
|
It doesn't look good to me that the behavior is platform dependent. I'll investigate how it happens later |
|
Please go ahead and merge. The only missing feature of VisualRegressionTests.jl that I find really useful is the GUI that pops up when tests fail. It makes it easy to quickly update test data. However, I agree that the updates occur too often due to other noises related to GTK, etc. |
Yes, currently it displays images using ImageInTerminal, which sometimes doesn't give a clear view. There's a pending PR in ReferenceTests that renders images using ImageShow: JuliaTesting/ReferenceTests.jl#41. That PR is pending because some mock tests are needed to detect any possible regressions. I plan to update that PR this month and then I believe you'd be satisfied. |
|
Oh that is really nice. In that case I will try to get rid of VisualRegressionTests.jl in other packages as well in the future. |
|
Did you finish this PR @johnnychen94 ? Please feel free to merge if it is ready. That should indeed help the other PRs being written. |
|
Not yet, the CI error is concerning and I'm not sure what's the cause here. It would be great if you can have a look at it since you're more familiar with the algorithm. I'm updating ReferenceTests these days to make it more stable. |
|
Of course, thank you for the heads up. Will try to take a look during the day. |
|
Is it my impression or the
I think it is just the image size when I plot with |
|
Also, is this information relevant? Should we set the number of CPU cores to 1 to get reproducible reference tests? |
|
I'm not sure if this is the same case; I just ran into a similar case that reference tests are broken when I'm not 100% sure that ReferenceTests is working correctly, though. Will try to fix it when I get some more time. |
|
@johnnychen94 did you succeed in the ReferenceTests.jl side? |
|
Not yet. Unfortunately, I was quite busy this summer to continue this because of a series of day-to-day seminars. There're some unidentified bugs about its faithfully and my very limited time was spent on adding more test cases If you have any plans on this package, please do it, I can just rebase on whatever you've done when I finally get here. |
|
Solved in #22 |
As @mgautam98 is going to improve this package with
AbstractArray{<:Colorant}support ( #13 ) , I'm doing some preparation work now. This one is for the test, and I'll open a PR for CI later.I'm still resistant to the usage of VisualRegressionTests for two reasons:
VisualRegressionTestsis still very large a dependency withPlotsandGtk.@juliohm Are you okay with this change?