Forbid new check warnings #2404

infotroph · 2019-09-04T13:37:58Z

Description

Experimental, opening PR for discussion but not confident this is the right approach. Opinions, please!

Increments the strictness of PEcAn's package checks by forbidding newly added WARNINGs or NOTEs without breaking the build for the (many) pre-existing messages.

In keeping with #1949, the idea here is to move toward eventually requiring all changes to maintain a clean devtools::check result: Many check messages indicate issues with potentially severe consequences even if are labeled as "WARNING" or "NOTE" rather than "ERROR", and it's preferable to to fix even the minor-looking issues so that we don't miss the severe ones in the noise.

Since #2174, all packages are checked during Travis builds and the build is marked broken if any ERRORs are found. It would be very simple to ratchet this up to break on any WARNING, but fixing all existing warnings from legacy code would be a big undertaking and we might accumulate new warnings faster than we fixed old ones.

What I proposed in #1949 was to blacklist a slowly growing list of warning types, e.g. fix all '::' or ':::' import not declared and break the build on new ones, then tackle Undocumented code objects, and so on. On consideration this still feels Sisyphean, so what I propose here is an across-the-board grandfather clause: We cache current check outputs and break the build on any message newly added by the current build, while continuing to let existing messages slide like we have been doing. This should enforce good practice in newly added code without forcing too much immediate cleanup in the old stuff. As historic warnings get cleaned up in future refactorings, we can update the cached check results to make sure they stay gone.

Motivation and Context

Review Time Estimate

Immediately
Within one week
When possible

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

My change requires a change to the documentation.
I have updated the CHANGELOG.md.
I have updated the documentation accordingly.
I have read the CONTRIBUTING document.
I have added tests to cover my changes.
All new and existing tests passed.

… new messages

infotroph · 2019-09-04T13:45:03Z

Sample of Travis output when a new problem is detected: https://travis-ci.org/infotroph/pecan/jobs/580354445#L6106-L6128

Apparently R check now does some static analysis and complains about "unstated dependencies in tests" because of these lines even though they never run. Easier to remove them than to fight it.

scripts/check_with_errors.R

ashiklom · 2019-09-17T20:49:07Z

It's still marked as "Draft", so not sure if a formal review is appropriate, but in general, this looks super useful -- thanks for putting it together!

infotroph · 2019-09-17T21:16:50Z

@ashiklom I marked this as draft to emphasize that I wanted feedback before it was merged, so reviews are very helpful.

If the overall approach seems sound, I'll make whatever other tweaks are needed to make the build pass and then solicit a final round of review.

… import

No idea why this complains on Travis but not locally, but whatever

base/workflow/R/create_execute_test_xml.R

ashiklom · 2019-09-18T13:59:36Z

I would like to see this go in sooner rather than later, especially given the flurry of PEcAn development that will be happening in anticipating of upcoming meetings. So feel free to drop the "Draft" status whenever you're ready so we can give this a formal review.

If you have the time, one thing that would help this PR is adding a brief (maybe even just in bullet form) section on "Continuous Integration / Travis" to our documentation. I think this would work as either an appendix, a sub-section of "Developer Guide -> Testing", or maybe even a full section under "Developer Guide". I would be perfectly happy with just a list of checks that are performed on Travis and a few notes about solving common errors (e.g. forgetting to run make document, fixing new "check" messages per this PR, re-starting the build if it times out).

R 3.5 reports the bare Rd filename, R >= 3.6 reports it as `man/<file>.Rd`. Decided it was easier to fix the error than to make the script handle it gracefully

robkooper · 2019-09-18T21:39:02Z

~~Do we need all the Rcheck_reference.log or those there as a what still needs to be fixed, or does the code use this to see if new warnings/errors are produced?~~

Saw where the files are needed.

infotroph · 2019-09-18T21:42:37Z

@robkooper We need them (the code compares each new check result against Rcheck_reference.log to find out what errors are new), but I'm not certain tests is the best place to store them and would welcome suggestions.

infotroph · 2019-09-18T21:44:25Z

base/workflow/R/create_execute_test_xml.R

-#' @return
+#' @return A list with two entries:
+#'  * `sys`: Exit value returned by the workflow (0 for sucess).
+#'  * `outdir`: Path where the workflow results are saved


@ashiklom please verify this is a sensible return type description

robkooper · 2019-09-18T21:44:42Z

scripts/check_with_errors.R

+    unlist(lapply(msg, function(x)paste(x[[1]], x[-1], sep=": ")))
+}
+
+old_file <- file.path(pkg, "tests", "Rcheck_reference.log")


This answers my own question about the Rcheck_reference.log

robkooper

I'd would suggest to commit. That way people can fix a WARNING/ERROR etc and commit it so it is easy to see warnings being fixed instead of having this ever changing pull request.

infotroph · 2019-09-18T21:59:46Z

modules/assim.sequential/R/sda.enkf_refactored.R

@@ -49,7 +49,7 @@ sda.enkf <- function(settings,
  host       <- settings$host
  forecast.time.step <- settings$state.data.assimilation$forecast.time.step  #idea for later generalizing
  nens       <- as.numeric(settings$ensemble$size)
-  processvar <- settings$state.data.assimilation$process.variance %>% as.logical()
+  processvar <- as.logical(settings$state.data.assimilation$process.variance)


@para2x I think my changes in this file are mostly in your code, especially from #2405 (it was merged after I created the reference files and therefore its check warnings aren't ignored). I tried to only remove pipes without changing code behavior, but please check that I didn't break anything.

infotroph · 2019-09-18T22:05:54Z

TODO before merging:

Get last few errors through Travis
Optionally, could punt to a separate PR: Documentation per @ashiklom request

ashiklom

This looks great, thanks again for your hard work on this!

book_source/02_demos_tutorials_workflows/05_developer_workflows/04-testing.Rmd

ashiklom · 2019-09-19T12:26:30Z

book_source/02_demos_tutorials_workflows/05_developer_workflows/04-testing.Rmd

+* Retrieves any cached files available from previous Travis builds.
+    - The main thing in the cache is previously-installed dependency R packages, to avoid recompiling them every time.
+    - If the cache becomes stale or is preventing a package update needed by the build (e.g. to get a new version that contains a needed bug fix), delete the cache through the Travis web interface and it will be reconstructed on the next build.
+    - Because PEcAn has so many dependencies, builds with no cache will spend most of their time recompiling packages and will often run out of time before the tests complete. You can fix this by [triggering a custom build](https://blog.travis-ci.com/2017-08-24-trigger-custom-build) using a custom config that installs but does not test PEcAn (e.g. `script: make install`), waiting for this to finish and upload its new cache, then restarting the standard full build.


This is an excellent suggestion. I did not know about this workaround.

mdietze

@infotroph I was about to merge this, but then I noticed all these new get.model.output.[MODEL] functions, all of which are dummy functions. What do these do and why are they being added?

ashiklom · 2019-09-19T12:47:20Z

@mdietze Unless I'm missing something, I only see one new get.model.output function in this PR -- get.model.output.dalec -- and it's just there to replace hand-written documentation about a non-existent get.model.output.generic function. We currently have a bunch of other get.model.output.[MODEL] functions (e.g. get.model.output.ED2, get.model.output.BIOCRO) that are real functions that do real things.

That said, for the get.model.output.dalec specifically, it may be worth either (1) having this function throw an error that says it hasn't yet been implemented, or (2) just deleting it altogether to avoid confusion.

infotroph · 2019-09-19T12:53:37Z

@mdietze the man/ directory for DALEC contained a hand-written Rd file whose \title was get.model.output.generic but its \usage showed get.model.output.dalec, leading to a check complaint about "code/documentation mismatch".

No idea why this showed up now but not in the stored check results file, but I figured the quickest fix was to add the dummy function so that Roxygen would regenerate the Rd with correct formatting (and with the correct filename).

I'm happy with either of @ashiklom's suggestions (add error or delete) and will do whichever the boss says.

…s/04-testing.Rmd Co-Authored-By: Alexey Shiklomanov <alexey.shiklomanov@gmail.com>

infotroph added 8 commits September 4, 2019 14:58

compare check results against previously cached version, error if any…

d868410

… new messages

cleanup

5598fca

first versions of all reference check logs

d7b0e71

remove file write, cleanup

447a83e

show new errors rather than make user guess

b34048a

use base funs

6bdb9b5

wording

03a9960

Merge branch 'develop' into forbid-new-check-warnings

0e66a61

ashiklom self-requested a review September 5, 2019 14:38

infotroph added 5 commits September 14, 2019 09:27

Merge branch 'develop' into forbid-new-check-warnings

f064d12

Merge branch 'develop' into forbid-new-check-warnings

69c515a

Merge branch 'develop' into forbid-new-check-warnings

4e70c01

Merge branch 'develop' into forbid-new-check-warnings

49ca6a1

remove dead code

9dd9085

Apparently R check now does some static analysis and complains about "unstated dependencies in tests" because of these lines even though they never run. Easier to remove them than to fight it.

ashiklom reviewed Sep 17, 2019

View reviewed changes

scripts/check_with_errors.R Outdated Show resolved Hide resolved

infotroph added 2 commits September 17, 2019 23:02

avoid direct DBI dependency in workflow

af82226

remove outdated workaround

74c6388

infotroph added 5 commits September 18, 2019 10:17

fix warnings added since I saved the reference file

168787b

typo

a7f4121

always report package name

2efcb70

functionalize 2-item pipes for easier reading and to avoid undeclared…

61b3c4b

… import

enquote stray apostrophe

dfea8a8

No idea why this complains on Travis but not locally, but whatever

ashiklom reviewed Sep 18, 2019

View reviewed changes

base/workflow/R/create_execute_test_xml.R Show resolved Hide resolved

ignore small differences in potential recompression savings

5a8d76b

infotroph added 2 commits September 18, 2019 16:30

ignore warning from existing dataset

5accc92

defancify quotes

a36d80a

infotroph added 2 commits September 18, 2019 19:49

ok, defancify AND strip commas

4cde04d

fix markup error rather than fight path differences

ac58098

R 3.5 reports the bare Rd filename, R >= 3.6 reports it as `man/<file>.Rd`. Decided it was easier to fix the error than to make the script handle it gracefully

infotroph marked this pull request as ready for review September 18, 2019 21:43

infotroph commented Sep 18, 2019

View reviewed changes

robkooper reviewed Sep 18, 2019

View reviewed changes

robkooper approved these changes Sep 18, 2019

View reviewed changes

infotroph commented Sep 18, 2019

View reviewed changes

infotroph added 4 commits September 19, 2019 00:58

replace hand-written Rd stub with Roxygen

005173f

remove stray trailing lines

5beba86

missing quote

dd5875a

document Travis checks

cb16f9c

ashiklom approved these changes Sep 19, 2019

View reviewed changes

ashiklom reviewed Sep 19, 2019

View reviewed changes

book_source/02_demos_tutorials_workflows/05_developer_workflows/04-testing.Rmd Outdated Show resolved Hide resolved

ashiklom reviewed Sep 19, 2019

View reviewed changes

book_source/02_demos_tutorials_workflows/05_developer_workflows/04-testing.Rmd Outdated Show resolved Hide resolved

ashiklom reviewed Sep 19, 2019

View reviewed changes

mdietze requested changes Sep 19, 2019

View reviewed changes

infotroph and others added 3 commits September 19, 2019 14:58

Update book_source/02_demos_tutorials_workflows/05_developer_workflow…

edb46d5

…s/04-testing.Rmd Co-Authored-By: Alexey Shiklomanov <alexey.shiklomanov@gmail.com>

Update book_source/02_demos_tutorials_workflows/05_developer_workflow…

96b8965

…s/04-testing.Rmd Co-Authored-By: Alexey Shiklomanov <alexey.shiklomanov@gmail.com>

remove dummy functions

2eac81f

mdietze approved these changes Sep 19, 2019

View reviewed changes

mdietze merged commit e0716a2 into PecanProject:develop Sep 19, 2019

infotroph deleted the forbid-new-check-warnings branch October 23, 2019 08:24

infotroph mentioned this pull request Nov 29, 2019

Timeline for package checks in CI (+ list of current check errors) #1949

Closed

8 tasks

robkooper added this to the 1.8.0 milestone Apr 14, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Forbid new check warnings #2404

Forbid new check warnings #2404

infotroph commented Sep 4, 2019 •

edited

infotroph commented Sep 4, 2019 •

edited

ashiklom commented Sep 17, 2019

infotroph commented Sep 17, 2019

ashiklom commented Sep 18, 2019

robkooper commented Sep 18, 2019 •

edited

infotroph commented Sep 18, 2019

infotroph Sep 18, 2019

robkooper Sep 18, 2019

robkooper left a comment

infotroph Sep 18, 2019

infotroph commented Sep 18, 2019 •

edited

ashiklom left a comment

ashiklom Sep 19, 2019

mdietze left a comment

ashiklom commented Sep 19, 2019

infotroph commented Sep 19, 2019 •

edited

Forbid new check warnings #2404

Forbid new check warnings #2404

Conversation

infotroph commented Sep 4, 2019 • edited

Description

Motivation and Context

Review Time Estimate

Types of changes

Checklist:

infotroph commented Sep 4, 2019 • edited

ashiklom commented Sep 17, 2019

infotroph commented Sep 17, 2019

ashiklom commented Sep 18, 2019

robkooper commented Sep 18, 2019 • edited

infotroph commented Sep 18, 2019

infotroph Sep 18, 2019

Choose a reason for hiding this comment

robkooper Sep 18, 2019

Choose a reason for hiding this comment

robkooper left a comment

Choose a reason for hiding this comment

infotroph Sep 18, 2019

Choose a reason for hiding this comment

infotroph commented Sep 18, 2019 • edited

ashiklom left a comment

Choose a reason for hiding this comment

ashiklom Sep 19, 2019

Choose a reason for hiding this comment

mdietze left a comment

Choose a reason for hiding this comment

ashiklom commented Sep 19, 2019

infotroph commented Sep 19, 2019 • edited

infotroph commented Sep 4, 2019 •

edited

infotroph commented Sep 4, 2019 •

edited

robkooper commented Sep 18, 2019 •

edited

infotroph commented Sep 18, 2019 •

edited

infotroph commented Sep 19, 2019 •

edited