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

Reporter output control via "file" argument or options setting (#629) #635

Merged
merged 9 commits into from Oct 10, 2017

Conversation

Projects
None yet
4 participants
@nealrichardson
Copy link
Contributor

nealrichardson commented Oct 7, 2017

Here's a solution that implements both the "testthat.output_file" option for all reporters, as well as allowing you to initialize a reporter directly with a "file" argument. The change for that in reporter.R is small, and most of the rest of the code changes are just passing ... around so that the "file" argument can be provided for all subclasses of Reporter.

A few open questions:

  • I didn't add any validation of the "file" argument input. As it stands, cat() will determine what it can write to and raise an error if it can't. I'm not sure it's worth adding validation in testthat that essentially duplicates that but with a different message wording, but happy to add it if you think it is worth doing.
  • When writing to a file, the reporter will always overwrite the file if it exists. Can add an "overwrite" argument (or "append", if you want to keep consistency with what cat takes), if you think it's worthwhile.
  • I left in the "testthat.junit.output_file" option for backwards compatibility, but since it hasn't actually been included in a released version (if I recall correctly), you may prefer to remove it entirely. It's easily enough removed if you'd like (deleting 1 line of code, 7 lines of test).
@hadley
Copy link
Member

hadley left a comment

We should definitely nix the special junit option.

What do you think about removing the option altogether?

@@ -34,7 +38,8 @@ Reporter <- R6::R6Class("Reporter",
warning("append ignored", call. = FALSE)
}

cat(..., file = self$out, sep = sep, fill = fill, labels = labels)
cat(..., file = self$out, sep = sep, fill = fill, labels = labels,

This comment has been minimized.

Copy link
@hadley

hadley Oct 10, 2017

Member

Can you please use tidyverse style here? http://style.tidyverse.org/syntax.html#long-lines

expect_identical(read_lines(output_file), read_lines(path))
}

output <- tempfile()

This comment has been minimized.

Copy link
@hadley

hadley Oct 10, 2017

Member

Could you mimic the approach of the "reporters produce consistent output" test? I think it's better to have one test, with some copy and paste inside of it. It's too hard to eliminate all the repetition here because of the varying arguments to the reporter.

nealrichardson added some commits Oct 10, 2017

@nealrichardson

This comment has been minimized.

Copy link
Contributor Author

nealrichardson commented Oct 10, 2017

Style changes made to code and tests, and I removed the testthat.junit.output_file option. Please take a look.

I did leave the new testthat.output_file option in because it doesn't add significant complexity---it's just getOption("testthat.output_file", stdout()) instead of stdout() as the default. And I do see the value of having it. As the commit message where the testthat.junit.output_file option was originally added (cb7cae0) says:

These are needed so that Ci systems can run test scripts in R packages
without modification, but first set these options() so that test output will
be written to a known location where it can be read and parsed.

Does that seem reasonable to you?

@hadley

This comment has been minimized.

Copy link
Member

hadley commented Oct 10, 2017

Ok, that seems fine.

@hadley
Copy link
Member

hadley left a comment

Looking good - a few more thoughts on the tests.

)
})

test_that("reporters write to 'testthat.output_file', if specified", {

This comment has been minimized.

Copy link
@hadley

hadley Oct 10, 2017

Member

I think this only needs one test since it's only one line of code

This comment has been minimized.

Copy link
@nealrichardson

nealrichardson Oct 10, 2017

Author Contributor

It was in testing all of the reporters that I found that several of them needed ... added to their initialize methods to pass the file argument through to super$initialize, so there is actually more than just the one line of code exercised by these tests.

This comment has been minimized.

Copy link
@hadley

hadley Oct 10, 2017

Member

I think that applies to the test case above, not this one? (Because the option is set in the base class initialiser)

This comment has been minimized.

Copy link
@nealrichardson

nealrichardson Oct 10, 2017

Author Contributor

Ah sorry, misread. You're right, will prune.


test_that("reporters accept a 'file' arugment and write to that location", {
output <- tempfile()
test_report_to_file("check", file = output, error_regexp=NULL, output_file = output)

This comment has been minimized.

Copy link
@hadley

hadley Oct 10, 2017

Member

You only need to test each reporter once, i.e. not multiple times for SummaryReporter

})


test_report_to_file <- function(name, reporter = find_reporter_one(name, ...),

This comment has been minimized.

Copy link
@hadley

hadley Oct 10, 2017

Member

I'd call this expect_...

@nealrichardson

This comment has been minimized.

Copy link
Contributor Author

nealrichardson commented Oct 10, 2017

Test function renamed and redundant SummaryReporter tests pruned.

@nealrichardson

This comment has been minimized.

Copy link
Contributor Author

nealrichardson commented Oct 10, 2017

Ok, redundant testthat.output_file tests pruned.

@hadley hadley merged commit 8a3a4d6 into r-lib:master Oct 10, 2017

0 of 2 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build failed
Details
@hadley

This comment has been minimized.

Copy link
Member

hadley commented Oct 10, 2017

Thanks! Much appreciated

@hadley

This comment has been minimized.

Copy link
Member

hadley commented Oct 10, 2017

Doh, just noticed the tests are failing 😞

@nealrichardson

This comment has been minimized.

Copy link
Contributor Author

nealrichardson commented Oct 10, 2017

Hmm, I had just merged upstream/master in and it was passing, so must be a very recent change/bad merge.

@nealrichardson

This comment has been minimized.

Copy link
Contributor Author

nealrichardson commented Oct 10, 2017

https://travis-ci.org/nealrichardson/testthat/builds/286231796 passed, and according to https://github.com/nealrichardson/testthat/tree/reporter-output-control it was 7 commits behind upstream/master. Let me know if I can help debug.

@hadley

This comment has been minimized.

Copy link
Member

hadley commented Oct 10, 2017

@hadley

This comment has been minimized.

Copy link
Member

hadley commented Oct 10, 2017

Hmmm, output_file seems to be getting a bunch of results written to it

@nealrichardson

This comment has been minimized.

Copy link
Contributor Author

nealrichardson commented Oct 10, 2017

Thinking aloud as I look at this. Looking at what TAP is printing out, it starts with

1..17
# Context Expectations
ok 1 Success
...

then after 17 starts over:

...
# Context Output
ok 16 Output:1
ok 17 Output:1
1..34
# Context Expectations
ok 1 Success
not ok 2 Failure:1
...

So it's like the tests are being run twice on the reporter.

FWIW I don't see this duplication outside of the test suite. If I run tests on a different package using this version of testthat, I don't get the duplicated test output.

@nealrichardson

This comment has been minimized.

Copy link
Contributor Author

nealrichardson commented Oct 10, 2017

Merging (git cherry-pick) in the commits from hadley/master that weren't in this branch one at a time and running the tests, 1b292a7 is when the tests start to fail.

@nealrichardson

This comment has been minimized.

Copy link
Contributor Author

nealrichardson commented Oct 10, 2017

Removing expect_silent from expect_report_to_file in test-reporter.R fixes the double test running (perhaps an interaction of expect_silent and expect_error in the context of 1b292a7?) and instead yields these failures, which if I understand correctly are related to the colorizing of the test output.

─ Failed ───────────────────────────────────────────────────────────────────────
1. Failure: reporters accept a 'file' arugment and write to that location (@test-reporter.R#114) 
`read_lines(output_file)` not identical to `read_lines(path)`.
7/59 mismatches
x[1]: "\033[31m1. Failure: Failure:1 (@tests.R#8) \033[39m----------------------
x[1]: -----------------------"
y[1]: "1. Failure: Failure:1 (@tests.R#8) --------------------------------------
y[1]: -------"

x[4]: "\033[31m2. Failure: Failure:2a (@tests.R#12) \033[39m--------------------
x[4]: -----------------------"
y[4]: "2. Failure: Failure:2a (@tests.R#12) ------------------------------------
y[4]: -------"

x[7]: "\033[31m3. Failure: Failure:2b (@tests.R#15) \033[39m--------------------
x[7]: -----------------------"
y[7]: "3. Failure: Failure:2b (@tests.R#15) ------------------------------------
y[7]: -------"

x[10]: "\033[31m4. Failure: Failure:loop (@tests.R#20) \033[39m------------------
x[10]: -----------------------"
y[10]: "4. Failure: Failure:loop (@tests.R#20) ----------------------------------
y[10]: -------"

x[15]: "\033[31m5. Error: Error:1 (@tests.R#28) \033[39m-------------------------
x[15]: -----------------------"
y[15]: "5. Error: Error:1 (@tests.R#28) -----------------------------------------
y[15]: -------"

2. Failure: reporters accept a 'file' arugment and write to that location (@test-reporter.R#115) 
`read_lines(output_file)` not identical to `read_lines(path)`.
25/114 mismatches
x[1]: "✔ | OK \033[31mF\033[39m \033[35mW\033[39m \033[34mS\033[39m | Context"
y[1]: "✔ | OK F W S | Context"

x[10]: "\033[31m✖\033[39m |  2 4     | Expectations"
y[10]: "✖ |  2 4     | Expectations"

x[12]: "\033[1mtests.R:8: \033[31mfailure\033[39m: Failure:1\033[22m"
y[12]: "tests.R:8: failure: Failure:1"

x[15]: "\033[1mtests.R:12: \033[31mfailure\033[39m: Failure:2a\033[22m"
y[15]: "tests.R:12: failure: Failure:2a"

x[18]: "\033[1mtests.R:15: \033[31mfailure\033[39m: Failure:2b\033[22m"
y[18]: "tests.R:15: failure: Failure:2b"

3. Failure: reporters accept a 'file' arugment and write to that location (@test-reporter.R#120) 
`read_lines(output_file)` not identical to `read_lines(path)`.
19/72 mismatches
x[1]: "Expectations: \033[32m.\033[39m\033[31m1\033[39m\033[31m2\033[39m\033[31m
x[1]: 3\033[39m\033[31m4\033[39m\033[32m.\033[39m"
y[1]: "Expectations: .1234."

x[2]: "Errors: \033[31m5\033[39m\033[31m6\033[39m"
y[2]: "Errors: 56"

x[3]: "Recursion: \033[31m7\033[39m"
y[3]: "Recursion: 7"

x[4]: "Skips: \033[34mS\033[39m\033[34mS\033[39m\033[34mS\033[39m"
y[4]: "Skips: SSS"

x[5]: "Warnings: \033[35mW\033[39m\033[35mW\033[39m\033[35mW\033[39m"
y[5]: "Warnings: WWW"

4. Failure: reporters accept a 'file' arugment and write to that location (@test-reporter.R#126) 
`read_lines(output_file)` not identical to `read_lines(path)`.
1/1 mismatches
x[1]: "\033[32m.\033[39m\033[31mF\033[39m\033[31mF\033[39m\033[31mF\033[39m\033[
x[1]: 31mF\033[39m\033[32m.\033[39m\033[31mE\033[39m\033[31mE\033[39m\033[31mE\0
x[1]: 33[39m\033[34mS\033[39m\033[34mS\033[39m\033[34mS\033[39m\033[35mW\033[39m
x[1]: \033[35mW\033[39m\033[35mW\033[39m.\033[32m.\033[39m"
y[1]: ".FFFF.EEESSSWWW.."

5. Failure: reporters write to 'testthat.output_file', if specified (@test-reporter.R#139) 
`read_lines(output_file)` not identical to `read_lines(path)`.
19/72 mismatches
x[1]: "Expectations: \033[32m.\033[39m\033[31m1\033[39m\033[31m2\033[39m\033[31m
x[1]: 3\033[39m\033[31m4\033[39m\033[32m.\033[39m"
y[1]: "Expectations: .1234."

x[2]: "Errors: \033[31m5\033[39m\033[31m6\033[39m"
y[2]: "Errors: 56"

x[3]: "Recursion: \033[31m7\033[39m"
y[3]: "Recursion: 7"

x[4]: "Skips: \033[34mS\033[39m\033[34mS\033[39m\033[34mS\033[39m"
y[4]: "Skips: SSS"

x[5]: "Warnings: \033[35mW\033[39m\033[35mW\033[39m\033[35mW\033[39m"
y[5]: "Warnings: WWW"

═ DONE ═════════════════════════════════════════════════════════════════════════
@hadley

This comment has been minimized.

Copy link
Member

hadley commented Oct 11, 2017

Hmmm might be related to #639 - I've clearly messed with the execution environment in a way I don't understand.

@hadley

This comment has been minimized.

Copy link
Member

hadley commented Oct 11, 2017

Doh - I was accidentally evaluating the input to expect_silent() twice 😭

hadley added a commit that referenced this pull request Oct 11, 2017

Fix bugs causing #635 to fail
* expect_silent() evaluating input twice
* need to manually disable coloured output
@hadley

This comment has been minimized.

Copy link
Member

hadley commented Oct 11, 2017

I've hacked around the issue with colours but it might be worth thinking about more - in general, should we be disabling colours in output when saving to a file, or should we expect the user will also disable that in options?

@nealrichardson

This comment has been minimized.

Copy link
Contributor Author

nealrichardson commented Oct 11, 2017

Great, glad you were able to track down the issues.

I'd think you wouldn't want colors in the file output. The reporters/*.txt files didn't have colors printed in them before, which means that we aren't expecting that capture_output/expect_known_file preserves colors, if I understand correctly. I'd expect the same for the direct file output for reporters.

Of course, writing reports to files is really for the machine-readable output formats (Junit, TAP, etc.), which don't colorize their output, so maybe it's not a big deal.

@gaborcsardi

This comment has been minimized.

Copy link
Member

gaborcsardi commented Oct 11, 2017

Yeah, I agree, colors should not go to files.

(Unfortunately, crayon has no way of knowing where the constructed strings are outputted, it just assumes stdout. If stdout is a file, it does not use ansi sequences.)

@akbertram

This comment has been minimized.

Copy link
Contributor

akbertram commented Dec 23, 2017

Sorry, I had added the original junit option but hadn't seen this conversation earlier. Thanks for preserving the option, it's certainly essential for builds on Jenkins and all of our Renjin testing infrastructure!

akbertram added a commit to bedatadriven/renjin that referenced this pull request Dec 30, 2017

Update TestMojo to match new testthat options
The "testthat.junit.output_file" option I had previously added
to testthat was changed to "testthat.output_file":
r-lib/testthat#635
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.