Add @test_reference_plot

[juliohm]

Fix #62

I need some help with the keyword options. I tried to pass them to the inner @test_reference call but got errors and couldn't fix them myself.

[juliohm]

If you would like, I can also move to the new Pkg approach with a separate test/Project.toml file. It is cleaner than this [extras] section.

[johnnychen94]

I think a better approach is to specialize _convert method for Plot types(AbstractPlot, maybe. I don't know), and use Requires.jl to conditionally load that specialization method when Plots.jl is loaded.

Also, it doesn't sound good to me to drop 1.0 support only to give Plots.jl support, which means any further API changes we planed in #58 will not be available in Julia 1.0. @oxinabox how do you think?

FWIW, I believe both PNGFiles(requires Julia >1.3) and ImageMagick(compatible to "all" Julia versions) allows an IO stream pipeline.

If you would like, I can also move to the new Pkg approach with a separate test/Project.toml file. It is cleaner than this [extras] section.

Please don't do this in this PR, let's keep this PR as simple as it can. Also do note that using a separate test/Project.toml requires Julia >= 1.2.

[juliohm]

I think adding a dependency just to get access to AbstractPlot is too much. The code I added doesn't need extra dependencies and works fine no?

On Thu, Feb 11, 2021, 14:26 Johnny Chen notifications@github.com wrote:

I think a better approach is to specialize _convert https://github.com/JuliaTesting/ReferenceTests.jl/blob/31deee3ee6134ef4b45b6417b61f78592776a71c/src/test_reference.jl#L110 method for Plot types(AbstractPlot, maybe. I don't know), and use Requires.jl https://github.com/JuliaPackaging/Requires.jl to conditionally load that specialization method.

Also, it doesn't sound good to me to drop 1.0 support only to give Plots.jl support, which means any further API changes we planed in #58 https://github.com/JuliaTesting/ReferenceTests.jl/issues/58 will not be available in Julia 1.0. @oxinabox https://github.com/oxinabox how do you think?

If you would like, I can also move to the new Pkg approach with a separate test/Project.toml file. It is cleaner than this [extras] section.

Please don't, let's keep this PR as simple as it can. Do note that using a separate test/Project.toml requires Julia >= 1.2.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/JuliaTesting/ReferenceTests.jl/pull/76#issuecomment-777659195, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAZQW3PNR6UAP7VYAGSSZELS6QHKTANCNFSM4XM3XZ2A .

[johnnychen94]

Why should we introduce a new API when the current test_reference pipeline already enables you to extend it?

[juliohm]

That is a good point. I'm just wondering if we need an extra dependency for dispatching with AbstractPlot

On Thu, Feb 11, 2021, 14:47 Johnny Chen notifications@github.com wrote:

Why should we introduce a new API when the current test_reference pipeline already enables you to extend it?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/JuliaTesting/ReferenceTests.jl/pull/76#issuecomment-777672569, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAZQW3MLPSPO7ZGANBSVK2LS6QJ3NANCNFSM4XM3XZ2A .

[johnnychen94]

I would be happier to have a clean design with a relatively small Requires.jl dependency; many packages(including Plots) load Requires so it won't be a big deal.

[juliohm]

Just a heads up that my experience with Requires.jl isn't very good. It doesn't allow control of compat versions so your optional dependencies change in a breaking way and you only know later when someone reports it. I'd simply introduce various macros like @test_plot @test_img and so on to avoid this dependency version issue.

On Thu, Feb 11, 2021, 15:11 Johnny Chen notifications@github.com wrote:

I would be happier to have a clean design with a relatively small Requires.jl dependency; many packages(including Plots) load Requires so it won't be a big deal.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/JuliaTesting/ReferenceTests.jl/pull/76#issuecomment-777687512, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAZQW3NMPOKWAJW7CWPUXPTS6QMVZANCNFSM4XM3XZ2A .

[johnnychen94]

We only need the type AbstractPlot and I don't think it will be changed in a foreseeable future. As I've said, the internal pipeline design already enables you to extend the support to Plots.

Also, the responsibility of a maintainer is to merge and fix upstream breaking changes, and not introduce breaking changes to downstream. We already have plans to deprecate the whole test_reference macro in favor of match_reference function, so adding a API that must be deprecated in short future is not a good option, especially when it's exported.

What's more, even if we need to introduce a new function, it should be asimage only. The new marco wrapper is definitely unnecessary.

To be more clear, I'd strongly reject this version if there's no other reviewer involved. These codes don't really need to exist in this package, it can be put in any package that you use. And it's totally acceptable to have some duplication for test only codes.

[juliohm]

These are all fair points. I'm just trying to help here with an initial draft. You know what is best for the package. I'm not involved in the development so I have no idea of the plans.

On Thu, Feb 11, 2021, 15:53 Johnny Chen notifications@github.com wrote:

We only need the type AbstractPlot and I don't think it will be changed in a foreseeable future. As I've said, the internal pipeline design already enables you to extend the support to Plots.

Also, the responsibility of a maintainer is to merge and fix upstream breaking changes, and not introduce breaking changes to downstream. We already have plans to deprecate the whole test_reference macro in favor of match_reference function, so adding a API that must be deprecated in short future is not a good option, especially when it's exported.

What's more, even if we need to introduce a new function, it should be asimage only. The new marco wrapper is definitely unnecessary.

To be more clear, I'd strongly reject this version if there's no other reviewer involved. These codes don't really need to exist in this package, it can be put in any package that you use. And it's totally acceptable to have some duplication for test only codes.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/JuliaTesting/ReferenceTests.jl/pull/76#issuecomment-777711676, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAZQW3LU3R2BOX3SUUDFS5LS6QRRBANCNFSM4XM3XZ2A .

[juliohm]

Just to reiterate my point of view here @johnnychen94 , I think you need to distinguish between what is an elegant design and what is a practical solution that addresses user needs. If you only aim for the first goal, you will likely spend more time than needed developing the package. If the design is too complex to maintain, only a few people will be aware of the dispatch rules and internal behavior, which will also limit future fixes by end users. Overdesign can be a problem as you know.

I tend to think of this effort here as a centralized development of a general isapprox for different objects. Whether this will lead to separate @test_img @test_plot @test_array that are easy to understand or to a single complex match_reference, that is up to the main developers, and also a function of how much time you are willing to spend evolving the code base when someone reports an issue. To me this package solves a very specific problem and doesn't need a sophisticated design. However, if you feel that it could become part of Julia Base or something bigger, then it makes sense to carefully evolve the design.

[oxinabox]

I am strongly against adding a Plots dependency, even behind Requires.

1. Plots adds a ton to load time, and causes weird errors on some CI systems unless you do a thing. Putting it behind Requires.jl does solve that.
2. Requires.jl significantly increases load-time, if you do need to load the package.
3. As mentioned Required.jl doesn't play nice with compat, while it seems safe to assume that Plots.jl won't have breaking changes to the existance of AbstractPlot, I still think its not worth it. We have enough similar weirdness to that from using FileIO

This is a nice solution because it doesn't care about Plots at all, just showable. calling it test_reference_plot is a bit of a minomer.

I think we might be able to do this without even adding a new command; or adding any new dependencies. I note that:

using Plots
using ImageIO  # loaded via FileIO.jl
using ReferenceTests

 @test_reference "temp.png" heatmap([1 0; 0 1])

will the first time it is called create the actual PNG that is wanted on disk.

Then calling it again (so it has a PNG top load) errors here: https://github.com/JuliaTesting/ReferenceTests.jl/blob/31deee3ee6134ef4b45b6417b61f78592776a71c/src/fileio.jl#L7-L9

With

 ERROR: MethodError: no method matching Plots.Plot{Plots.GRBackend}(::Matrix{RGB{FixedPointNumbers.N0f8}})
Closest candidates are:
  Plots.Plot{T}(::Any, ::Any, ::Any, ::Any, ::Any, ::Any, ::Any, ::Any, ::Any, ::Any) where T<:AbstractBackend at /Users/oxinabox/.julia/packages/Plots/6EMd6/src/types.jl:68
Stacktrace:

Note that the type is Matrix{RGB{FixedPointNumbers.N0f8}} which AIUI is how a correctly loaded image is represented in the Images.jl ecosystem. The error message is that you can;t call Plots.Plot{Plots.GRBackend} on it. But actually you don't need to because it is already in the right form?

So maybe all we need to do is remove that T? maybe it is just a mistake to have it there?

 function loadfile(::Type{T}, file::File) where T
    T(load(file))::T # Fallback to FileIO
end

Or maybe we need to have a fallback:

function loadfile(::Type{T}, file::File) where T
    # Fallback to FileIO
    data = load(file)
    try
        T(data)
    catch
        data
    end
end

I don't have time to investigate more right now. But it seems very promising that the temp.png file is being created (and almost loaded) correctly.

I think a better approach is to specialize _convert method for Plot types(AbstractPlot, maybe. I don't know), and use Requires.jl to conditionally load that specialization method when Plots.jl is loaded.

We might need to overload _convert but not to specialize on AbstractPlot, but rather on: ::Type{DataFormat{:PNG}}

Rather than adding dependency on PNGFiles at all we might just want to write to a tempfile and read it back in.

function _convert(::Type{<:DataFomat{:PNG}}, data}
    mktempdir() do dir
        filename = File{DataFomat{:PNG}}(joinpath(dir, "inconversion.png"))
        savefile(filename, data)
        load(filename)
    end
end

Tempfiles often go to a RAM Disk anyway

---

Please don't do this in this PR, let's keep this PR as simple as it can. Also do note that using a separate test/Project.toml requires Julia >= 1.2.

I agree. Dropping 1.0 support would be bad.

[juliohm]

It is not clear to me what could be a temporary solution to this issue. Can you please advise on how I could update the PR so that it gets merged and so that we don't need to copy the asimage everywhere in our packages? I have at least 10 packages that are now using ReferenceTests.jl

[oxinabox]

update the PR to make @test_reference do this without needing a new macro with a different name. This can be done with appropriate changes to loadfile and _convert so that they can handle thing with DataFormat{:PNG} and generic object types (which will include AbstractPlot). and avoid adding a PNGFiles dependancy by writing to a tempfile instead.

[oxinabox]

Closing given #77