775 implement knit print for subclasses of cohortsize including tests #784
Conversation
…ncluding-tests' of github.com:Roche/crmPack into 775-implement-knit_print-for-subclasses-of-cohortsize-including-tests
Unit Tests Summary 1 files 40 suites 3m 55s ⏱️ Results for commit b629f79. ♻️ This comment has been updated with latest results. |
Unit Test Performance Difference
Additional test case details
Results for commit 01941b3 ♻️ This comment has been updated with latest results. |
…ncluding-tests' of github.com:Roche/crmPack into 775-implement-knit_print-for-subclasses-of-cohortsize-including-tests # Conflicts: # tests/testthat/test-helpers_knitr.R
…ncluding-tests' of github.com:Roche/crmPack into 775-implement-knit_print-for-subclasses-of-cohortsize-including-tests
…ncluding-tests' of github.com:Roche/crmPack into 775-implement-knit_print-for-subclasses-of-cohortsize-including-tests
|
Code Coverage SummaryDiff against mainResults for commit: b629f79 Minimum allowed coverage is ♻️ This comment has been updated with latest results |
…ncluding-tests' of github.com:Roche/crmPack into 775-implement-knit_print-for-subclasses-of-cohortsize-including-tests
|
Test coverage report suggests that |
There was a problem hiding this comment.
Thanks @Puzzled-Face , nice work!
I have some questions here about the tests... can we maybe stick to a simpler, even though more lines of code involved, way of handling this?
tests/testthat/test-helpers_knitr.R
Outdated
| crmpack_class_list <- setdiff(crmpack_class_list, exclusions) | ||
|
|
||
| # See https://stackoverflow.com/questions/42738851/r-how-to-find-what-s3-method-will-be-called-on-an-object | ||
| identifyMethod <- function(generic, ...) { |
There was a problem hiding this comment.
hmm........ this will be difficult to maintain. any way we can keep this much simpler for the tests?...
There was a problem hiding this comment.
I'm not sure which part you think will be difficult to maintain: the exclusion list, the code to "find" the S3 method for each class, or something else?
The exclusion list is awkward, I agree. But I think it's reasonably stable - it's essentially a list of the virtual classes that crmPack defines. I don't see that changing very frequently. I propose two ways of handling this in #734. [Note the paragraph that reads
An alternative approach would be to require all methods to exist even for virtual superclasses. The actual method implementation for these virtual superclasses would then throw an exception (as is done by the default constructors for these super classes. This would simplify the unit testing, but at the cost of additional code that is never expected to be used.
I used the same approach to test that all the necessary tidy methods had been defined.
[As an aside, I think one weakness of our current tests is that we only test that what we have done works correctly. We don't test that we have done everything we should have done. The approach I've taken here is, I think, the only way to address that weakness.]
If you mean that the code to find the appropriate S3 method is flaky, I disagree. The fundamental process should hold true regardless of what happens in the future. If not, it's likely that S3 is broken and we will have bigger issues to deal with!
If you mean the use of is_checking(), I think that can probably be removed now. It was one of the things I tried to solve the problems I asked about in chat. I think Craig spotted the root cause, so this is probably unnecessary now.
The use of if (methodName != "knit_print.default") is effectively temporary. Once we have all the necessary knit_print methods implemented, we can either remove it entirely, or (my preference) replace it with fail(paste0("Required knit_print method has not been defined for ", cls)) or similar. There's a step to do this listed in #774. Again, I took the same approach when testing the tidy methods.
There was a problem hiding this comment.
I can add more comments if you think that would be helpful.
There was a problem hiding this comment.
Basically the whole lines 22-63 are difficult to maintain. I don't understand what the code is doing to be honest. Plus there is a lot of typically dangerous things that work on the language, e.g. deparse , substitute, assign.
My question is would it be possible to have much more vanilla kind of testing here, which is avoiding those dangerous things, avoiding loops where possible, and keep with a maybe tedious but explicit list of tests?
Example: https://github.com/Roche/crmPack/blob/main/tests/testthat/test-Model-class.R I guess we could
have tried to use loops to avoid manual writing of tests here too, but we kept with the much more simple structure.
If it is possible - let's go with the more simple approach. If not - then we need to derisk this code, by testing it explicitly (yes, testing the code that is used for tests here), documenting, commenting, etc.
There was a problem hiding this comment.
OK. I hope I can reassure you. The code that makes you uncomfortable is temporary. Put simply, it says "do we have a custom knit_print method for the current class? If so, test it. If not, do nothing.". So, once we (think we) have knit_print methods for every crmPack class, we can delete the conditional testing and just test everything.
In this particular case, we could delete the conditional testing immediately. That will mean that tests will fail when we introduce new knit_print methods, but I don't see that as a major problem: we'd have to inspect new snapshot files in that case anyway.
The more important reason for keeping the current structure is that I can re-use it in other situations. For example, to test the behaviour of the asis parameter to knit_print for crmPack classes. The generic knir_print doesn't have this, so we would need either to use a class-specific test (which I think is less reliable and definitely less efficient) or to use a similar approach.
I'll refactor to make the code more self-explanatory.
There was a problem hiding this comment.
Ok and let's try to also have tests for the test code, e.g. make a function for pieces of it, add documentation and tests. Otherwise somebody else cannot maintain this in the future
There was a problem hiding this comment.
Done. See revisions in the latest push
Merge branch '775-implement-knit_print-for-subclasses-of-cohortsize-including-tests' of github.com:Roche/crmPack into 775-implement-knit_print-for-subclasses-of-cohortsize-including-tests # Conflicts: # R/helpers_knitr_CohortSize.R
Puzzled-Face
deleted the
775-implement-knit_print-for-subclasses-of-cohortsize-including-tests
branch
Implements #775, part 1 of #774.