Fix CI #1689

tkf · 2018-08-23T21:23:55Z

I just blindly replace old API to the new one. Not sure if all the upstream packages are ready but I think it's handy if we can check Travis to see how much of them are ready.

tkf · 2018-08-24T04:05:31Z

test/travis_commands.jl

+Pkg.add([
+    PackageSpec("ImageMagick"),
+    # PackageSpec("GR"),
+    PackageSpec(url="https://github.com/tkf/PlotReferenceImages.jl", rev="pkg3"),


Revert a585592 once JuliaPlots/PlotReferenceImages.jl#30 is merged

tkf · 2018-08-24T04:51:49Z

test/add_packages.jl

+# ENV["PYTHON"] = ""
+
+Pkg.add([
+    PackageSpec(url="https://github.com/tkf/PlotReferenceImages.jl", rev="pkg3"),


TODO for me: Revert 1748063 once JuliaPlots/PlotReferenceImages.jl#30 is merged

tkf · 2018-08-24T04:57:48Z

Oops, I didn't mean to go this deep into tweaking your testing setup. I thought it was just a quick small fix :) I hope I didn't mess up anything.

The biggest change is that non-registered test requirements are now Pkg.added in test/runtests.jl. This is required for adding those packages inside the testing environment instead of the default global environment. I guess this is the easiest choice if you want to use Pkg.test but avoid checking in Manifest.toml. (see also JuliaDiff/ForwardDiff.jl#332 (comment))

Anyway, I think now the CI is reporting meaningful errors and I will not to try fixing them.

mkborregaard · 2018-08-24T05:35:56Z

I'm not really sure what the purpose is of this PR - could you explain? I think our existing setup quite closely followed the recommended default travis script for pacakges.

tkf · 2018-08-24T05:45:36Z

I fixed the test suite and CI setup so that they work with Julia 1.0 / Pkg3. I don't think the current setup in master branch is compatible with Julia 1.0. I should have edited the title since it does not describe this PR well any more.

tkf · 2018-08-24T06:06:33Z

Some more info: I don't think installing non-registered packages in travis_commands.jl as you were doing is a valid approach if you want to use Pkg.test:

The tests are run by generating a temporary environment with only pkg and its (recursive) dependencies (recursively) in it. If a manifest exist, the versions in that manifest is used, otherwise a feasible set of package are resolved and installed.

--- https://docs.julialang.org/en/latest/stdlib/Pkg/#Pkg.test

I can think of three choices:

Add non-registered packages in runtests.jl as I did here.
Add Project.toml and Manifest.toml (which is already supported by the default script). Manifest.toml can contain repo-url pointing to a non-registered git repository. The downside is that Manifest.toml has to be updated manually to reflect upstream changes.
Skip Pkg.test and directly invoke julia test/runtests.jl. The downside is that you have to install everything in test/REQUIRE manually.

Maybe there are more solutions. But I thought the first choice was reasonable.

daschw · 2018-08-24T09:18:59Z

Nice work @tkf! I actually have a local branch starting a quite similar implementation, but you are one step ahead.
We still need to finalize updating VisualRegressionTests.jl, but in the future it will optionally depend on Gtk. If the tests are run interactively (i.e. locally by a user) we want a Gtk dependency, so that the user can compare possible changes in the plot examples in a GUI window.
My first suggestion would be to also add Gtk to the dependencies and add something like

if isinteractive()
    using Gtk
end

This, however, also requires the Gtk dependency for CI runs, where it is not actually used. So if you have a better idea, I'm open to suggestions.
Otherwise, I'd ask you to add something similar to my code snippet above somewhere in e.g. runtests.jl, and we can revisit, once JuliaPlots/VisualRegressionTests.jl@1cba462 is released.

daschw · 2018-08-24T09:21:23Z

I can think of three choices:

I prefer 1.

tkf · 2018-08-25T00:09:36Z

Why doesn't @eval Main import Gtk (or maybe @static if isinteractive()?) work?

tkf · 2018-08-25T00:18:07Z

test/add_packages.jl

+    # PackageSpec(name="Blink", rev="master"),
+    # PackageSpec(name="Rsvg", rev="master"),
+    # PackageSpec(name="PlotlyJS", rev="master"),
+    PackageSpec(name="VisualRegressionTests", rev="master"),


TODO: Comment out above (revert a8e3ecc)

tkf · 2018-08-25T01:23:32Z

test/imgcomp.jl

@@ -2,6 +2,10 @@
 using VisualRegressionTests
 # using ExamplePlots

+if isinteractive()
+    @eval Main import Gtk


So it seems this part was ignored in Travis as intended https://travis-ci.org/JuliaPlots/Plots.jl/builds/420347392?utm_source=github_status&utm_medium=notification

Is it enough? Does it cover the use-case you have in mind?

tkf · 2018-08-25T02:46:09Z

So one of the problem with if isinteractive() is that it's not true in the subprocess launched by Pkg.test. It means that the user have to manually run("test/runtests.jl") if s/he wants to test Gtk related components (which may not be so bad?). Another option would be to check (say) ENV["PLOTS_TEST_GUI"] and install Gtk in test/add_packages.jl.

tkf · 2018-08-25T03:15:13Z

test/runtests.jl

@@ -1,5 +1,6 @@
 module PlotsTests

+include("add_packages.jl")


I feel bad about unconditionally installing packages via runtests.jl which could ruin users' environments. Something like

if isinteractive() @info """ Skip installing unregistered packages. To run Plots.jl test suite in interactive session, you need to install them manually by, e.g., `include("test/add_packages.jl")`. Alternatively, you can run the test with `Pkg.test("Plots")`. """ else include("add_packages.jl") end

would be nice. However, this still is not enough when the test is invoked from shell like julia test/runtests.jl. Checking Base.HOME_PROJECT and Base.ACTIVE_PROJECT may solve this problem but I guess we need to be careful not to exclude their values when invoked via Pkg.test.

mkborregaard · 2018-08-25T10:10:55Z

It means that the user have to manually run("test/runtests.jl") if s/he wants to test Gtk related components (which may not be so bad?).

Yes, that's the idea. Advanced users and people making PRs can run the tests manually to overwrite images with new versions, people just running test to know if this works on julia-someversion will just get an exception (the GTK window is only launched upon test errors)

mkborregaard · 2018-08-25T10:12:31Z

src/Plots.jl

@@ -116,7 +116,7 @@ ignorenan_minimum(x) = Base.minimum(x)
 ignorenan_maximum(x::AbstractArray{F}) where {F<:AbstractFloat} = NaNMath.maximum(x)
 ignorenan_maximum(x) = Base.maximum(x)
 ignorenan_mean(x::AbstractArray{F}) where {F<:AbstractFloat} = NaNMath.mean(x)
-ignorenan_mean(x) = Base.mean(x)


Maybe Statistics.mean to be explicit?

Makes sense. Applied in 6d0d8ea

mkborregaard · 2018-08-25T10:14:07Z

test/imgcomp.jl

@@ -15,6 +19,8 @@ using VisualRegressionTests

 using Plots
 # using StatPlots
+using PlotReferenceImages


Maybe just import PlotReferenceImages? You're just using it for pathof, right?

mkborregaard · 2018-08-25T13:09:25Z

You're on fire @tkf , great with all the contribs lately!

tkf · 2018-08-25T14:22:20Z

It was just a little hack in the beginning :)

If @daschw is OK with #1689 (comment) then I think it's good to go.

@daschw But If you want to do the fix in #1693 please feel free to close this issue and cherry pick some patches if some of them are useful.

daschw · 2018-08-26T18:12:13Z

No, I think I will go with this implementation. Seems to work better than my attempt in #1693. I will add my VisualRegressionTests related fixes in a separate PR.
Thanks so much for this @tkf!

Use Pkg3 API in travis_commands.jl

def5be6

tkf force-pushed the pkg3 branch from e034e5b to def5be6 Compare August 23, 2018 21:27

Fix appveyor.yml

7fcd008

tkf mentioned this pull request Aug 24, 2018

Make pathof(PlotreferenceImages) work JuliaPlots/PlotReferenceImages.jl#30

Merged

tkf commented Aug 24, 2018

View reviewed changes

tkf force-pushed the pkg3 branch 2 times, most recently from 0e95c44 to 5eae423 Compare August 24, 2018 04:42

tkf commented Aug 24, 2018

View reviewed changes

tkf changed the title ~~Use Pkg3 API in travis_commands.jl~~ Fix CI Aug 24, 2018

tkf added 7 commits August 24, 2018 17:14

Import Random

4a51e4e

Import Pkg

374580a

Use pathof(PlotReferenceImages) instead of Pkg.dir

80da14c

Install unregistered packages in test/runtests.jl

e483ce8

Add FileIO to test/REQUIRE

531dd43

Don't use Base.mean

eefea15

Import Gtk in interactive session

ebf3239

tkf force-pushed the pkg3 branch from 5eae423 to a8e3ecc Compare August 25, 2018 00:17

tkf commented Aug 25, 2018

View reviewed changes

mkborregaard reviewed Aug 25, 2018

View reviewed changes

mkborregaard mentioned this pull request Aug 25, 2018

WIP: make tests pass #1693

Closed

tkf added 2 commits August 25, 2018 07:16

Use qualified name Statistics.mean

6d0d8ea

Use import PlotReferenceImages

e0f9313

tkf force-pushed the pkg3 branch from a8e3ecc to e0f9313 Compare August 25, 2018 14:16

daschw merged commit 25190d9 into JuliaPlots:master Aug 26, 2018

daschw mentioned this pull request Aug 26, 2018

Fix tests #1700

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix CI #1689

Fix CI #1689

tkf commented Aug 23, 2018

tkf Aug 24, 2018

tkf Aug 24, 2018

tkf commented Aug 24, 2018 •

edited

Loading

mkborregaard commented Aug 24, 2018

tkf commented Aug 24, 2018

tkf commented Aug 24, 2018

daschw commented Aug 24, 2018

daschw commented Aug 24, 2018

tkf commented Aug 25, 2018

tkf Aug 25, 2018

tkf Aug 25, 2018

tkf commented Aug 25, 2018

tkf Aug 25, 2018

mkborregaard commented Aug 25, 2018

mkborregaard Aug 25, 2018

tkf Aug 25, 2018

mkborregaard Aug 25, 2018

mkborregaard commented Aug 25, 2018

tkf commented Aug 25, 2018

daschw commented Aug 26, 2018

Fix CI #1689

Fix CI #1689

Conversation

tkf commented Aug 23, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tkf commented Aug 24, 2018 • edited Loading

mkborregaard commented Aug 24, 2018

tkf commented Aug 24, 2018

tkf commented Aug 24, 2018

daschw commented Aug 24, 2018

daschw commented Aug 24, 2018

tkf commented Aug 25, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tkf commented Aug 25, 2018

Choose a reason for hiding this comment

mkborregaard commented Aug 25, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkborregaard commented Aug 25, 2018

tkf commented Aug 25, 2018

daschw commented Aug 26, 2018

tkf commented Aug 24, 2018 •

edited

Loading