revdepcheck icon indicating copy to clipboard operation
revdepcheck copied to clipboard

False negative

Open jennybc opened this issue 1 year ago • 4 comments

I'm in the process of releasing glue and CRAN's revdep checks picked up something that, upon manual scrutiny, is also present in my revdepcheck results, but that was not surfaced as a problem for me to inspect.

I'm wondering: should we have caught this or is it beyond our ambitions?

The revdep is lvmisc and there are 2 test failures which I do need to look at (and which I learned about via my CRAN submission).


Side note: in our cloud checks, lvmisc has a bunch of problems in examples and tests that have something to do with Matrix:

Error in initializePtr() : 
  function 'cholmod_factor_ldetA' not provided by package 'Matrix'

There are the same in old and new in cloud checks. They don't show up on CRAN. So that's creating some noise, but is not truly part of our story. But maybe it's related to why the real stuff didn't surface?


Here's the relevant excerpt of glue/revdep/cloud.noindex/.../lvmisc/old/lvmisc.Rcheck. All the test failures are due to this Matrix issue.

 > test_check("lvmisc")
  [ FAIL 8 | WARN 8 | SKIP 6 | PASS 225 ]
  
  ══ Failed tests ════════════════════════════════════════════════════════════════
  ── Error ('test-accuracy_method.R:54:3'): accuracy() returns a data frame with the right columns ──
  Error in `initializePtr()`: function 'cholmod_factor_ldetA' not provided by package 'Matrix'
 ...
  ── Error ('test-compare_accuracy.R:33:3'): compare_accuracy() works for models of class lmerMod ──
  Error in `initializePtr()`: function 'cholmod_factor_ldetA' not provided by package 'Matrix'
  ...
  ── Error ('test-compare_accuracy.R:68:3'): compare_accuracy() works for models of different classes ──
  Error in `initializePtr()`: function 'cholmod_factor_ldetA' not provided by package 'Matrix' ...
AND SO ON AND SO FORTH

Here's the comparable excerpt of glue/revdep/cloud.noindex/.../lvmisc/new/lvmisc.Rcheck. The Matrix-related failures are there plus 2 other failed tests that are relevant to the release.

 > test_check("lvmisc")
  [ FAIL 10 | WARN 8 | SKIP 6 | PASS 223 ]
  
   ══ Failed tests ════════════════════════════════════════════════════════════════
  ── Failure ('test-abort.R:77:3'): abort_no_method_for_class() works ────────────
  err$message (`actual`) not equal to glue::glue("The method `my_fun` is not yet implemented \\\n      for an object of class `my_class`.") (`expected`).
  
  lines(actual) vs lines(expected)
    "The method `my_fun` is not yet implemented for an object of class `my_class`."
  - ""
  ── Failure ('test-abort.R:109:5'): abort_no_method_for_class() works in objects with multiple classes ──
  err$message (`actual`) not equal to glue::glue("The method `my_fun` is not yet implemented \\\n       for an object of classes `my_class_1` and `my_class_2`.") (`expected`).
  
  lines(actual) vs lines(expected)
    "The method `my_fun` is not yet implemented for an object of classes `my_class_1` and `my_class_2`."
  - ""
  ── Error ('test-accuracy_method.R:54:3'): accuracy() returns a data frame with the right columns ──
  Error in `initializePtr()`: function 'cholmod_factor_ldetA' not provided by package 'Matrix'
... ALL THE PREVIOUSLY SEEN Matrix-related FAILURES ARE HERE

jennybc avatar Jan 09 '24 17:01 jennybc

Yeah, I think the current comparison is not so great, and consider these two to be the same. We could certainly try to improve it, but it is quite challenging, as we are comparing noisy strings.

gaborcsardi avatar Jan 09 '24 17:01 gaborcsardi

I can imagine that diffing the test results is not a small task. But maybe picking up on this would be both useful and not terribly brittle:

> test_check("lvmisc")
  [ FAIL 8 | WARN 8 | SKIP 6 | PASS 225 ]

vs

> test_check("lvmisc")
  [ FAIL 10 | WARN 8 | SKIP 6 | PASS 223 ]

Feel free to close if you don't foresee going here any time soon. Or leave open if it might be considered.

jennybc avatar Jan 09 '24 19:01 jennybc

AFAIR we do have the testthat output, I mean, the full output file by file, test by test, and the failed expectations, etc. So we could compare that. Let leave this open.

gaborcsardi avatar Jan 10 '24 09:01 gaborcsardi

Follow-up: when I looked at these test failures, I realized I had detected them months ago, in a previous revdep run. (And I had already made a PR to affected package, which cleared the way for my CRAN submission, yay.)

I think, at the previous revdep run, it was a matter of 0 test failures (old glue) vs. 2 test failures (new glue) and revdepcheck surfaced this in a transparent way. Therefore I do think the nuisance failures related to Matrix are part of this story. I.e. we recognize that 0 test failures is different from 2, but we don't recognize that 8 is different from 10.

jennybc avatar Jan 11 '24 02:01 jennybc