rework `default_rendermode` and add tests for `render` #66

johnnychen94 · 2020-08-01T07:15:40Z

This is a sub PR of #61

The previous default_rendermode(format, data) is problematic that the render pipeline is showing the original data instead of the encoded data.

ReferenceTests.jl/src/test_reference.jl

Line 115 in 30aa43f

render(rendermode, raw_actual)

For example, setting default_rendermode(::SHA256, img) to BeforeAfterLimited doesn't make much sense to me.

johnnychen94 · 2020-08-01T07:19:41Z

test/references/string/string5.txt

@@ -1,2 +1,2 @@
-This is a


This was used to check CRLF/LF difference, but given that #64 properly handles this test case, we no longer need it.

The new reference is automatically generated by rm -rf test/references.

The previous `default_rendermode(format, data)` is problematic that the render pipeline is showing the original data instead of the encoded data. That said, `default_rendermode(::SHA256, img)` is wrongly set to `BeforeAfterFull` -- we shoud never print the whole image array.

johnnychen94 · 2020-08-01T07:24:45Z

test/references/render/BeforeAfterFull/img1d_1_compare.txt

+- REFERENCE -------------------
+Gray{Normed{UInt8,8}}[Gray{N0f8}(0.0), Gray{N0f8}(0.102), Gray{N0f8}(0.2), Gray{N0f8}(0.298), Gray{N0f8}(0.4), Gray{N0f8}(0.502), Gray{N0f8}(0.6), Gray{N0f8}(0.698), Gray{N0f8}(0.8), Gray{N0f8}(0.902)]
+-------------------------------
+- ACTUAL ----------------------
+Gray{Normed{UInt8,8}}[Gray{N0f8}(0.902), Gray{N0f8}(0.8), Gray{N0f8}(0.698), Gray{N0f8}(0.6), Gray{N0f8}(0.502), Gray{N0f8}(0.4), Gray{N0f8}(0.298), Gray{N0f8}(0.2), Gray{N0f8}(0.102), Gray{N0f8}(0.0)]
+-------------------------------


How each pixel is rendered may change from versions to versions, in that case, the test will fail, but we may just regenerate these.

Cref: JuliaGraphics/ColorTypes.jl#202

It seems that images are not properly encoded by ImageInTerminal: I get different results in macOS and Ubuntu

johnnychen94 · 2020-08-01T10:13:25Z

test/testutils.jl

+function string_check(ref, actual)
+    # a over-verbose collection of patterns that we want to ignore during test
+    patterns = [
+        # Julia v1.6
+        "Normed{UInt8,8}" => "N0f8",
+        r"Array{(\w+),2}" => s"Matrix{\1}",
+        r"Array{(\w+),1}" => s"Vector{\1}",
+
+        # https://github.com/JuliaGraphics/ColorTypes.jl/pull/206
+        # r"Gray{\w+}\(([\w\.]+)\)" => s"\1",
+        # r"RGB{\w+}\(([\w\.,]+)\)" => s"RGB(\1)",
+    ]
+
+    for p in patterns
+        actual = replace(actual, p)
+        ref = replace(ref, p)
+    end
+
+    # Julia v1.4
+    ref = join(map(strip, split(ref, "\n")), "\n")
+    actual = join(map(strip, split(actual, "\n")), "\n")
+
+    isequal(ref, actual)
+end


Perhaps we can make this the default equality checking function for AbstractArray{<:Number} types. I'm not sure.

It doesn't fully support nested parameters, but I think [^,] is slightly better than \w.

julia> occursin(r"Array{(\w+),1}", "Array{Base.TwicePrecision{Float32},1}") false julia> occursin(r"Array{([^,]+),1}", "Array{Base.TwicePrecision{Float32},1}") true

Also, \b may be helpful.

julia> replace("MyLittleArray{Float32,2}", r"Array{(\w+),2}" => s"Matrix{\1}") "MyLittleMatrix{Float32}" julia> replace("MyLittleArray{Float32,2}", r"\bArray{(\w+),2}" => s"Matrix{\1}") "MyLittleArray{Float32,2}"

Of course, since the function is only for testing, my suggestion is not important. TBH, I don't really like modifying "reference"s. I prefer the approach by means of multiple references (#47)

oxinabox · 2020-08-03T12:27:14Z

src/render.jl

-function render_item(::BeforeAfterLimited, item)
-    show(IOContext(stdout, :limit=>true, :displaysize=>(20,80)), "text/plain", item)
-    println()
+render_item(mode::RenderMode, item) = render_item(stdout, mode, item)


I think we might not actually need this one anymore (or in the first place)
since render takes care of making sure we alrways have a IO arg

Suggested change

render_item(mode::RenderMode, item) = render_item(stdout, mode, item)

oxinabox · 2020-08-03T13:20:41Z

src/render.jl

 end

+# We set the fallback as limited mode because it is not safe/efficient to fully render anything unless
+#   * we have prior information that it is not long -- numbers


but if it is not long, then BeforeAfterLimitted and BeforeAfter will be the same

oxinabox · 2020-08-03T13:21:55Z

src/test_reference.jl

-    if rendermode === nothing
-        rendermode = default_rendermode(F, raw_actual)
-    end
+    rendermode === nothing && (rendermode = default_rendermode(T))


Can't we just change line 97 to be:
rendermode = default_rendermode(T)?

oxinabox · 2020-08-03T13:23:24Z

test/render.jl

+        for (x, xname) in (
+                        #    (img1d_1, "img1d_1"),
+                        #    (img1d_2, "img1d_2"),
+                        #    (img1d_3, "img1d_3"),


we should leave a comment saying why these are commented out

oxinabox · 2020-08-03T13:24:09Z

test/render.jl

+
+end
+
+# `render_item` is repeatly called by `render` so we can skip it


Suggested change

# `render_item` is repeatly called by `render` so we can skip it

# `render_item` is an internal helper called by `render` so we don't test it directly

oxinabox · 2020-08-03T13:24:42Z

test/render.jl

+using ReferenceTests: Diff, BeforeAfterFull, BeforeAfterImage, BeforeAfterLimited
+using ReferenceTests: render, render_item
+
+refdir = joinpath(refroot, "render")


Suggested change

refdir = joinpath(refroot, "render")

const refdir = joinpath(refroot, "render")

oxinabox · 2020-08-03T13:26:21Z

src/render.jl

 """
-    default_rendermode(::DataFormat, actual)
+    default_rendermode(actual)


It nolonger gets the actual, only its type?

Suggested change

default_rendermode(actual)

default_rendermode(typeof(actual))

oxinabox · 2020-08-03T13:27:46Z

src/render.jl

+default_rendermode(::Type) = BeforeAfterLimited()
+default_rendermode(::Type{T}) where T<:Number = BeforeAfterFull()
+default_rendermode(::Type{T}) where T<:AbstractString = Diff()
+default_rendermode(::Type{T}) where T<:AbstractArray{<:AbstractString} = Diff()
+default_rendermode(::Type{T}) where T<:AbstractArray{<:Colorant} = BeforeAfterImage()


Suggested change

default_rendermode(::Type) = BeforeAfterLimited()

default_rendermode(::Type{T}) where T<:Number = BeforeAfterFull()

default_rendermode(::Type{T}) where T<:AbstractString = Diff()

default_rendermode(::Type{T}) where T<:AbstractArray{<:AbstractString} = Diff()

default_rendermode(::Type{T}) where T<:AbstractArray{<:Colorant} = BeforeAfterImage()

default_rendermode(::Type) = BeforeAfterLimited()

default_rendermode(::Type{<:Number}) = BeforeAfterFull()

default_rendermode(::Type{<:AbstractString}) = Diff()

default_rendermode(::Type{<:AbstractArray{<:AbstractString}}) = Diff()

default_rendermode(::Type{<:AbstractArray{<:Colorant}}) = BeforeAfterImage()

johnnychen94 commented Aug 1, 2020

View reviewed changes

johnnychen94 force-pushed the jc/render_test branch from cc9c3a9 to 2689c14 Compare August 1, 2020 07:21

johnnychen94 commented Aug 1, 2020

View reviewed changes

fix nightly test failure

b4347cc

johnnychen94 force-pushed the jc/render_test branch from 7b0f356 to b4347cc Compare August 1, 2020 08:42

johnnychen94 added 2 commits August 1, 2020 17:16

skip failed test cases

224ac3b

It seems that images are not properly encoded by ImageInTerminal: I get different results in macOS and Ubuntu

fix Julia < 1.4 test failure

1d77c05

johnnychen94 commented Aug 1, 2020

View reviewed changes

oxinabox reviewed Aug 3, 2020

View reviewed changes

johnnychen94 mentioned this pull request Aug 11, 2020

support multiple references #47

Open

kimikage mentioned this pull request Aug 13, 2020

Implement < in addition to isless JuliaGraphics/ColorTypes.jl#213

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rework `default_rendermode` and add tests for `render` #66

rework `default_rendermode` and add tests for `render` #66

johnnychen94 commented Aug 1, 2020 •

edited

Loading

johnnychen94 Aug 1, 2020

johnnychen94 Aug 1, 2020

johnnychen94 Aug 1, 2020

kimikage Aug 10, 2020 •

edited

Loading

oxinabox Aug 3, 2020

oxinabox Aug 3, 2020

oxinabox Aug 3, 2020

oxinabox Aug 3, 2020

oxinabox Aug 3, 2020

oxinabox Aug 3, 2020

oxinabox Aug 3, 2020

oxinabox Aug 3, 2020


		end

		# `render_item` is repeatly called by `render` so we can skip it

	# `render_item` is repeatly called by `render` so we can skip it
	# `render_item` is an internal helper called by `render` so we don't test it directly

	refdir = joinpath(refroot, "render")
	const refdir = joinpath(refroot, "render")

	default_rendermode(actual)
	default_rendermode(typeof(actual))

rework default_rendermode and add tests for render #66

Are you sure you want to change the base?

rework default_rendermode and add tests for render #66

Conversation

johnnychen94 commented Aug 1, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kimikage Aug 10, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rework `default_rendermode` and add tests for `render` #66

rework `default_rendermode` and add tests for `render` #66

johnnychen94 commented Aug 1, 2020 •

edited

Loading

kimikage Aug 10, 2020 •

edited

Loading