ci: switch to GitHub Actions #37
Conversation
- coverage display wasn't set up before, so leaving that alone in the readme for now
|
I'm going to remove the review request until you sort out the CI failures, but please let me know if you need any help troubleshooting. |
There was a problem hiding this comment.
Snapshot tests for this branch pass for me on latest Metworx, but fail with main (f518cdb), so I think changes in b20ce83 are good.
$ Rscript -e 'devtools::test()'
ℹ Testing pmforest
✔ | F W S OK | Context
✔ | 27 | base-plot [11.0s]
✔ | 3 | input-data
✔ | 2 | nsim-plot [1.7s]
✔ | 5 | summary
══ Results ═══════════════════════════════════════════════════════════════════════════════════════════════════════════
Duration: 13.4 s
[ FAIL 0 | WARN 0 | SKIP 0 | PASS 37 ]
$ git rev-parse HEAD
b20ce83cc8b67b491a6ac6dc096253074bd567b4
R/helpers-for-tests.R
Outdated
| @@ -1,4 +1,4 @@ | |||
| #' Skip test if not on Metworx or Drone | |||
| #' Skip test if not on Metworx or CI | |||
There was a problem hiding this comment.
Hmm, this description seems incorrect to me. This helper skips if PMF_SKIP_VDIFFR is set to true, but the description is "Skip test if not on Metworx or CI". That's misleading because that environment variable isn't inherently tied to either Metworx or the CI. And that's demonstrated by the fact that, before this PR, that variable was set in DRONE and this skip was triggered, while the variable is not set in these GitHub Actions and the skip is not triggered.
How about changing this to "Skip test if `PMF_SKIP_VDIFFR` is set to true"?
Also, given the description of skip_vdiffr says "[t]his is useful because vdiffr::expect_doppleganger() is very fragile and tends to fail with false positives in other environments (like CI and R CMD CHECK)", should GitHub Actions set this? Or, given the actions are passing without the skip, should we wait and see how fragile these end up being?
Side remark: the commit title (37eb86b) for this change is "tests helpers: prefer CI env var to drone", but this doesn't look at the CI environment variable.
There was a problem hiding this comment.
You know I will say I expected the oldest ci action to fail without this variable set. vdiffr differences can cause mismatches in svgs, so I find it odd that oldest passed in both b57855d and b20ce83.
I'll adjust the documentation, but am leaning towards:
given the actions are passing without the skip, should we wait and see how fragile these end up being?
- This environment variable is no longer set in CI. Based on previous findings, this was necessary to avoid differences in SVG/snapshot expectation when installing various versions of `vdiffr` in CI. - This doesnt seem to be necessary now. The reasoning is unclear, but opting to _not_ set the environment variable in `.github/workflows/main.yaml` unless necessary.
No description provided.