Improved vdiffr setup #2856
Improved vdiffr setup #2856
Conversation
|
Should we keep it on r-devel? This way we can monitor changes to graphics devices etc.
I found these tests to be dependent on system versions of sf dependencies (gdal, geos). If possible, they should be changed (for instance to use fabricated data) so they are not affected by these system libraries. |
|
If sf tests depend on gdal and geos (makes sense, now that I think about it) then anything involving a coordinate transformation or other calculation on geospatial data (e.g., centroid calculation, graticules) is likely going to be unstable. We can probably visually test basic drawing of simple features but not much else. I’ll let @hadley make the call on whether visual tests should run on r-devel. |
|
The problem with checking on R devel is that any breaking r devel change will show up on a unrelated diff. I think testing with r release is likely to cause fewer false positives |
|
I have fixed the broken reference image and otherwise left the PR as is. I'll leave the problem with sf tests for now; I don't know how to resolve it. This means the PR is ready to be merged from my side. |
|
@hadley I was thinking getting the r-devel failures is desirable because it allows us to monitor the effects of upstream changes on ggplot2. |
|
Yes, but we get them in a very un-informative way - it will all depend on the timing of when the travis request is kicked off. If we cared about r-devel changes (which I don't know if we do), it would be better to do a daily run of CRAN ggplot2 against latest r-devel |
.travis.yml
Outdated
| matrix: | ||
| include: | ||
| - r: 3.1 | ||
| env: SKIP_VDIFFR=true |
There was a problem hiding this comment.
Can we flip this around to make skipping the default, so we only set VDIFFR=true for R-release?
There was a problem hiding this comment.
Yes, however that requires another environmental variable. We want the visual tests to run when manually testing, e.g. in R Studio, so the default has to be on. I can add a variable that indicates a CI run, and make the presence of that variable turn vdiffr off unless VDIFFR=true is also set.
There was a problem hiding this comment.
Oh that makes sense. Maybe we could turn off by default when the TRAVIS (IIRC) env var is set?
There was a problem hiding this comment.
I find having a separate variable we define slightly preferable, because that means vdiffr runs can be fully controlled in any testing environment where these tests may be run, whether on travis or not.
There was a problem hiding this comment.
I was thinking we could do both - if USE_VDIFFR is set, use its value, otherwise look at TRAVIS and don't run vdiffr if set.
There was a problem hiding this comment.
Ok, this seems to be working now. I put the broken image back in as a check to see which builds run vdiffr.
.travis.yml
Outdated
| env: SKIP_VDIFFR=true | ||
| - r: oldrel | ||
| env: SKIP_VDIFFR=true | ||
| - r: release |
There was a problem hiding this comment.
Since you're in here, would you mind re-organsing so the versions are in reverse chronological order? That way the most recent versions (which are mostly likely to reveal problems) will be run first
| env: | ||
| global: | ||
| # don't treat missing suggested packages as error | ||
| - _R_CHECK_FORCE_SUGGESTS_=false |
There was a problem hiding this comment.
It just occurred to me that we could set USE_VDIFFR to false here, and then I think the specific matrix env var would override this one.
|
This old issue has been automatically locked. If you believe you have found a related problem, please file a new issue (with reprex) and link to this issue. https://reprex.tidyverse.org/ |
As discussed under #2842. I have purposefully committed a breaking visual test, so if everything is right with this initial PR then the release build will fail and all others will succeed.