Tests don't work on travis

[daattali]

I don't think the visual tests ever ran on travis because the check for it was:

if (Sys.getenv("RunGgplot2Tests=yes") == "yes") runTests <- TRUE

After fixing it to

if (Sys.getenv("RunGgplot2Tests") == "yes") runTests <- TRUE

the build fails at the tests https://travis-ci.org/daattali/ggExtra/builds/285191191

@crew102 sorry to summon you, but do you have an idea?

[crew102]

Yeah, they never ran on travis. They were getting run on my feature PR and failing, i think b/c ggplot2 doesn't get installed. not sure why though.

[daattali]

Trying to recruit help :) https://twitter.com/daattali/status/917839201055526913

[daattali]

@crew102 did you have to do any special setup to get the tests to run properly for you locally? Did you just install the latest version of the required packages, and running devtools::test() works as expected?

[daattali]

@crew102 did you have to do any special setup to get the tests to run properly for you locally? Did you just install the latest version of the required packages, and running devtools::test() works as expected?

BTW travis seems to have one common error to what I was getting locally, about the deps.txt file: https://travis-ci.org/daattali/ggExtra/builds/286231357#L1277 lines 1277

[crew102]

Pretty sure it's an issue with changes in vdiffr. Specifically, the function that writes deps.txt changed recently:

https://github.com/lionel-/vdiffr/blob/1711da3407cda52372aea7f91d75badbdf90e312/R/cases.R#L109

As far as I can tell, you don't need to write deps.txt (it writes it for you). I just pushed a branch that should confirm this...I'm guessing that it won't pass for other reasons (i.e., the figs no longer match in some cases), but I'm thinking this takes care of the deps.txt issue.

Also FYI, if you look at the marginal mapping feature PR, you'll see that I re-organized the code that checks for whether tests should be run.

[daattali]

I've been trying to make sure I'm able to run the tests before merging this. If I can't resolve it, I'll merge before I can run the tests... are you not using the latest CRAN vdiffr?

[crew102]

I wasn't using the latest version of vdiffr when they were working for me a few weeks ago. I get the same error re: deps.txt when I test in a docker container using a similar environment to what travis uses...going back to the old version fixes the deps.txt issue, but tests still fail for what looks like several reasons.

[crew102]

I think we need Sys.unsetenv('R_TESTS') in there somewhere, so we can install ggplot2..that will at least solve one of the issues. there are others though.

[daattali]

I tried doing that Sys.unsetenv('R_TESTS') earlier and then removed it. I'll add it again now that it's using old vdiffr

[daattali]

Still getting errors, but they're more encouraging now: figures don't match:. That's a step in the right direction :)

[crew102]

think i finally got it....do you mind if i integrate these changes in my next pr?

https://github.com/daattali/ggExtra/pull/66/commits/6f143727904659634613db2bb788e172ad933fc4

[daattali]

This line that generates the expected figures: https://github.com/daattali/ggExtra/commit/6f143727904659634613db2bb788e172ad933fc4?w=1#diff-c37975432a5b2e463ef854f4e62cf9f6R56

That should be run manually only whenever you need to create new expected figs. But what's stopping it from being run every time the tests are run, I don't see it surrounded by an if?

I'd also consider adding to the README a final small section that describes how to contribute - how to run the tests (need to add the system variable) and how to generate the expected figs, because these things are not trivial and not documented

[daattali]

Do you want to add these changes right now instead of in an existing PR, so that we see if they work instead of integrating them into a larger PR?

[crew102]

That should be run manually only whenever you need to create new expected figs. But what's stopping it from being run every time the tests are run, I don't see it surrounded by an if?

That file actually needs to get run each time the tests are run.. it calls functions that produce plots, which are passed to vdiffr while the tests are being run so that up-to-date versions of the actual figs can be created...these actual figs are then compared to the expected figs, which are created in the one-off file render-figs.R (which needs to get re-sourced each time new figs are added to test cases).

It's confusing, I know. I still don't see any other alternative workflow for this, though. I agree re: need for contributing guide. Are you thinking a section in README or an actual CONTRIBUTING?

Do you want to add these changes right now instead of in an existing PR, so that we see if they work instead of integrating them into a larger PR?

Yeah, I want to get the tests straightened out first, then we can talk about the feature that is added in #66. I was thinking of opening up one more PR with the refactor of the test logic and small fix that is now resulting in tests passing on my local.

[daattali]

As long as the instructions for contributions are just 2-3 sentences, I'd prefer to add it at the end of the README. Only if it gets long I'd move it to a separate file. Yep please open a PR for fixing testing!