Skip to content
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

test_that() fails in interactive mode when there are several failures within the same expectation. #279

Closed
antoine-lizee opened this issue Jul 15, 2015 · 3 comments

Comments

@antoine-lizee
Copy link

@antoine-lizee antoine-lizee commented Jul 15, 2015

> test_that("everything goes normal", {
+     expect_equal(as.character(5), 5)
+ })
Error in vapply(failures, as.character, character(1)) : 
  values must be length 1,
 but FUN(X[[1]]) result is length 2

> get_reporter()$failures[[1]]
Not expected: as.character(5) not equal to 5
Modes: numeric, character. Not expected: as.character(5) not equal to 5
target is numeric, current is character. 

From what I tracked down, there are several problems:

  1. the "worst" one is that failures don't get cleaned up in the reporter if the function end_test() throws an error, which prevents further use of the package. A quick fix would be to flush it first.
    Then, either:
  2. format.expectation (called by as.character) should collapse the several failure_msg of the expectation.
  3. Only one failure_msg should be allowed.
@antoine-lizee antoine-lizee changed the title test_that() fails in interactive mode when there is several failures within the same expectation. test_that() fails in interactive mode when there are several failures within the same expectation. Jul 15, 2015
antoine-lizee added a commit to antoine-lizee/testthat that referenced this issue Jul 15, 2015
@antoine-lizee
Copy link
Author

@antoine-lizee antoine-lizee commented Jul 15, 2015

This first PR solves 1. and 2. in a pretty stupid way. I still don't know about 3. Are failures supposed to have several messages?

antoine-lizee added a commit to antoine-lizee/testthat that referenced this issue Jul 16, 2015
@antoine-lizee
Copy link
Author

@antoine-lizee antoine-lizee commented Jul 16, 2015

This PR is the right fix. The problem was coming from compare.numeric that was behaving differently than compare.default (the fallback of compare.character).

My diagnostic repex:

str(testthat:::compare.default("35", 4)) # compare.character() -> NextMethod() -> compare.default()
str(testthat:::compare.numeric(35, "4")) # 2 MESSAGES !

Should I add a better failover for the end_test() method of the reporter? This still could be a problem later on. (point (1.) above) I could keep something along the lines of my first PR or have something like a

tryCatch({...}, 
         finally = {failures <<- list()}
)

instead. Or just let it be :-).

@antoine-lizee
Copy link
Author

@antoine-lizee antoine-lizee commented Sep 24, 2015

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
1 participant