update to testthat 3e + consolidate translate() tests
#699
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
if `context()` wasn't redundant with file name, convert to comment
mostly just switching to `expect_equal` and updating attributes as needed. it also seems that `expect_warning()` (and other prompt friends) return the prompt rather than output--changed one of these that was related to `expect_equivalent, but will need to do a more thorough sweep when switching to `expect_snapshot`
`quo` does the trick in most of these situations, and any exceptions ought to be transitioned to `expect_snapshot`. still a good few errors related to function environment.
I think that it helps to standardize indentation + simplify figuring out what a snapshot is capturing, and it cuts down on lines quite a bit, but I could also see the argument that it obscures a bit too much. Implemented primary argument checks for three models, dropping the rest.
it looks like just about every model type has associated changes to translation of primary arguments, so pulling back from the "selected methods" approach to testing each.
also transitioned on output of `translate`
following Hannah's lead--we just want to make sure we get a model object back, and `expect_snapshot` will pick up differences in fit time etc
a good bit of `expect_warning` output changes affecting assignments. some namespacing, some environment variable tomfoolery as well.
ended up running into issues with `expect_snapshot` call environments—not worth the loss in interpretability of errors to wrap `expect_snapshot` itself. still trying to figure out the best way to ignore function environments here.
wipe the environment from quosures in the translation helper so that `expect_snapshot` is reproducible!
for whatever reason, `oldrel-2` can't find `lung` inside of the tests here. thinking this might be some kind of scoping issue—from what i can tell on the `survival` repo, this shouldn't be due to changes in that package, though it's hard to trace source history from there.
for `oldrel-2` and older, pak uses survival 3.1-8 via R 3.6.3. in these versions of survival, loading the lung data with `data(lung` seems to have worked (though was not preferred? ¯\_(ツ)_/¯)
This comment was marked as resolved.
This comment was marked as resolved.
topepo
requested changes
Apr 12, 2022
exceptions: * when assigning (i.e. no `extract_fit_*<-()`) * when subsetted into `$method` * when testing `NULL`
topepo
reviewed
Apr 19, 2022
This was referenced Apr 20, 2022
|
This pull request has been automatically locked. If you believe you have found a related problem, please file a new issue (with a reprex: https://reprex.tidyverse.org) and link to this issue. |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
A few things worth mentioning:
hpc <- hpc_data[1:150, c(2:5, 8)]object among all tests. A line intestthat/setup.Rseems like it would do the trick, except it makes interactively debugging tests a bit more clunky.expect_equal(…, list(…)idiom.test_thatcall, as 1) they share the same inputs and 2) with the newexpect_snapshot(translate_args())idiom, the difference between the two is more visually apparent.update()and bad input tests are mostly untouched. Both of those are relevant here, I think, but this PR is quite large already.load(test_path("mars_model.RData"))intest_marsmore discouraged in 3e? Idioms like this seem to live elsewhere in the tidymodels, so I just ran with it.