- 
                Notifications
    
You must be signed in to change notification settings  - Fork 340
 
Expect shape #1469
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Expect shape #1469
Conversation
| 
           It's been a while, but do you want to have a go now at finishing this off?  | 
    
| 
           SGTM, I've just pulled everything up to latest   | 
    
        
          
                tests/testthat/test-expect-shape.R
              
                Outdated
          
        
      | 
               | 
          ||
| # testing dim() | ||
| expect_success(expect_shape(matrix(nrow = 5, ncol = 4), c(5L, 4L))) | ||
| expect_failure(expect_shape(matrix(nrow = 6, ncol = 3), c(6L, 2L))) | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few of these should use expect_snapshot_failure() so we can see the failure messages.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have a good sense of when to use one vs. the other; at least one snapshot_failure() for each of the arguments makes sense to me superficially. I can add more plain expect_failure() if you see fit.
| 
           Learning interesting things about R every day :) foo = function(dim) dim(cbind(1:2))
foo()
# Error in dim(cbind(1:2)) : argument "dim" is missing, with no default | 
    
…ct return values & corresponding tests
| expect_length_impl_(act, n) | ||
| } | ||
| 
               | 
          ||
| expect_length_impl_ <- function(act, n) { | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is a case where you need to pass trace_env = parent.frame() to expect(). It is slowly starting to come back to me, and I'll document properly soon 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! I'll make a few small tweaks when I'm not on a 🛩️,
| 
           I have merged this, resisting the temptation to wait until October 4 🤣  | 
    
* upstream/HEAD: (250 commits) Implement `expect_shape()` (r-lib#1469) Re-enable catch tests on windows (r-lib#2104) Move digest to suggests (r-lib#2105) Support emscripten in `skip_on_os()` (r-lib#2103) Add `skip_unless_r()` (r-lib#2094) Require R 4.1 (r-lib#2101) Allow unquoting first arg for `expect_s4_class()` (r-lib#2065) typo fix (r-lib#2051) Explicitly pass `parent.frame()` in `it()` (r-lib#2086) Update vignette advice for migrating to {testthat} 3e (r-lib#2080) New praise messages (r-lib#1974) move '!' outside aggregation (r-lib#2067) Handle deprecation of std::uncaught_exception() (r-lib#2097) perf: use `anyNA()` (r-lib#2058) Upkeep (r-lib#2099) Implement `mock_output_sequence()` (r-lib#2061) Increment version number to 3.2.3.9000 Increment version number to 3.2.3 Update CRAN comments Finish off r-lib#2046 ...
| 
           Excited to see this on CRAN!  | 
    
Closes #1423