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

Use conditions for reporting results #360

Merged
merged 36 commits into from Feb 19, 2016

Conversation

Projects
None yet
2 participants
@krlmlr
Member

krlmlr commented Feb 5, 2016

Also: test_that() returns TRUE if successful (no failed expectations, errors, or skips) for programmatic use of testthat.

Bare expectations now simply stop if a test fails. The stop occurs upon the first event, unlike the StopReporter which stops after the first failed test.

@krlmlr krlmlr changed the title from test_that() returns if tests passed to test_that() returns TRUE if successful (no failed expectations, errors, or skips) Feb 5, 2016

Kirill Müller added some commits Feb 5, 2016

Kirill Müller
class = c("test_result", "condition"))
withRestarts(
if (results$passed) signalCondition(cond) else stop(cond),

This comment has been minimized.

@krlmlr

krlmlr Feb 5, 2016

Member

Could also call message() instead of signalCondition() here.

@krlmlr

This comment has been minimized.

Member

krlmlr commented Feb 5, 2016

krlmlr pushed a commit to r-dbi/DBItest that referenced this pull request Feb 5, 2016

Kirill Müller
Merge branch 'feature/return-test-results'
- Return test results as named array of logical. Requires r-lib/testthat#360.

krlmlr pushed a commit to r-dbi/DBItest that referenced this pull request Feb 6, 2016

Kirill Müller
Merge tag 'v1.0-5'
- New tweak `union`, for specifying a nonstandard way of combining queries.
- All union queries now use full qualification for each column (required for `bigrquery`).
- Return test results as named array of logical. Requires r-lib/testthat#360.
- Use fork of `testthat`.

krlmlr pushed a commit to r-dbi/DBItest that referenced this pull request Feb 10, 2016

withRestarts(
{
signalCondition(cond)

This comment has been minimized.

@krlmlr

krlmlr Feb 10, 2016

Member

Call stop() here.

@krlmlr krlmlr changed the title from test_that() returns TRUE if successful (no failed expectations, errors, or skips) to Use conditions for reporting results Feb 12, 2016

Kirill Müller added some commits Feb 12, 2016

Kirill Müller
Kirill Müller
update tests
Now a failing expectation always raises an error, even if the StopReporter isn't active. The update of this test reflects this change in behavior.

krlmlr pushed a commit to r-dbi/DBItest that referenced this pull request Feb 12, 2016

Kirill Müller
Merge tag 'v1.1'
- New feature: tweaks
    - New argument `tweaks` to `make_context()` (#49).
    - New `tweaks()`, essentially constructs a named list of tweaks but with predefined and documented argument names.
    - `constructor_name`, respected by the `constructor.*` tests.
    - `strict_identifier`, if `TRUE` all identifier must be syntactic names even if quoted. The quoting test is now split, and a part is ignored conditional to this tweak. The `roundtrip_quotes` tests also respects this tweak.
    - `omit_blob_tests` for DBMS that don't have a BLOB data type.
    - `current_needs_parens` -- some SQL dialects (e.g., BigQuery) require parentheses for the functions `current_date`, `current_time` and `current_timestamp`.
    - `union`, for specifying a nonstandard way of combining queries. All union queries now name each column in each subquery (required for `bigrquery`).

- New tests
    - `dbGetInfo(Result)` (r-dbi/DBI#55).
    - `dbListFields()` (#26).
    - New "package_name" test in `test_getting_started()`.
- Improved tests
    - Stress test now installs package in temporary library (before loading `DBI`) using `R CMD INSTALL` before loading DBI (r-dbi/RSQLite#128, #48).
    - Row count is now tested for equality but not identity, so that backends can return a numeric value > 2^31 at their discretion.
    - Call `dbRemoveTable()` instead of issuing `DROP` requests, the latter might be unsupported.
    - Use subqueries in queries that use `WHERE`.
    - Test that `dbClearResult()` on a closed result set raises a warning.
    - Expect a warning instead of an error for double disconnect (#50).
    - Move connection test that requires `dbFetch()` to `test_result()`.
    - Split "can_connect_and_disconnect" test.
    - Expect `DBI` to be in `Imports`, not in `Depends`.
- Removed tests
    - Remove test for `dbGetException()` (r-dbi/DBI#51).
- Bug fixes
    - Fix broken tests for quoting.
- Self-testing
    - Test `RPostgres`, `RMySQL`, `RSQLite` and `RKazam` as part of the Travis-CI tests (#52).
    - Travis CI now installs rstats-db/DBI, updated namespace imports (`dbiCheckCompliance()`, `dbListResults()`).

    - Use fork of `testthat`.
- Utilities
    - Return test results as named array of logical. Requires r-lib/testthat#360, gracefully degrades with the CRAN version.
- Internal
    - Refactored the `get_info_()` tests to use a vector of names.
    - Use versioned dependency for DBI
    - Use unqualified calls to `dbBind()` again
- Same as 1.0-8.

@krlmlr krlmlr referenced this pull request Feb 18, 2016

Closed

FR: DebugReporter #356

@@ -73,6 +82,28 @@ test_code <- function(description, code, env) {
invisible(ok)
}
report_results <- function(results) {

This comment has been minimized.

@hadley

hadley Feb 18, 2016

Member

I think this would be better called report_expectation(), but I'm not sure report() is quite the right verb (but I'm not sure what would be better)

This comment has been minimized.

@hadley

hadley Feb 18, 2016

Member

Or should this all be wrapped up into expectation()?

This comment has been minimized.

@krlmlr

krlmlr Feb 18, 2016

Member

Do you mean that expectation() should create and raise the condition object? This means that we need to be very clever in expect_that(): Catch the condition (which is created in the condition(object) call), patch it, and re-raise it. To me, this looks like too much effort for too little gain. But I agree that expectation() should return a proper condition object right away. I'll update the PR soon.

This comment has been minimized.

@hadley

hadley Feb 18, 2016

Member

Maybe we should have expectation() (noun) to create the object, and expect() (verb) raise the condition?

This comment has been minimized.

@krlmlr

krlmlr Feb 18, 2016

Member

We already have expect() which accepts a logical and a message; we'll need as.expectation.logical() and as.expectation.expectation() to avoid changing too much unrelated code, but otherwise this is doable. How does that sound?

This comment has been minimized.

@krlmlr

krlmlr Feb 18, 2016

Member

Then we'll add as.expectation.error(), as.expectation.skip() (as replacement for current expecation_error() and expectation_skipped()) and later as.expectation.warning() (#310).

raise_condition_from_result <- function(results) {
if (results$passed) {
cond <- structure(list(results = results),

This comment has been minimized.

@hadley

hadley Feb 18, 2016

Member

Could we combine this object representation with the expectation, so there's only one type of thing to worry about?

@krlmlr

This comment has been minimized.

Member

krlmlr commented Feb 19, 2016

Are you planning further refactorings? Then it would be great to merge this beforehand, otherwise I'll work further on this branch.

Kirill Müller
@hadley

This comment has been minimized.

Member

hadley commented Feb 19, 2016

I don't have anything else major planned - I think most of the other issues need this PR completed first.

Kirill Müller
@krlmlr

This comment has been minimized.

Member

krlmlr commented Feb 19, 2016

Extracted expectation_ok(), got rid of success_msg in my code, added two TODO markers.

I think test_code() can be simplified considerably, so that there's basically just one handler function taking care of everything. The reporter should be able to decide if tryCatch() catches the errors or not (#356) :

tryCatch(stop(), error = NULL)
tryCatch(stop(), error = function(e) NULL)

I'll merge the other PR before adding NEWS to this PR.

update_expectation(exp, srcref)
}
update_expectation <- function(exp, srcref, info = NULL, label = NULL) {

This comment has been minimized.

@hadley

hadley Feb 19, 2016

Member

It feels like we should be able to eventually eliminate this function, wrapping it into the constructor

This comment has been minimized.

@krlmlr

krlmlr Feb 19, 2016

Member

Difficult without finally deprecating info and label arguments (#218).

This comment has been minimized.

@hadley

hadley Feb 19, 2016

Member

Yeah, lets look again once we've eliminated those args and refactored the expectations

@@ -52,28 +52,55 @@ test_code <- function(description, code, env) {
e$calls <- utils::head(sys.calls()[-seq_len(frame + 7)], -2)
signalCondition(e)
}
handle_expectation <- function(exp) {
get_reporter()$add_result(exp)
if (!expectation_ok(exp)) {

This comment has been minimized.

@hadley

hadley Feb 19, 2016

Member

ok <<- expectation_ok(exp) ?

This comment has been minimized.

@krlmlr

krlmlr Feb 19, 2016

Member

ok <<- ok && expectation_ok(exp) if you prefer.

}
raise_condition <- function(exp) {
if (exp$passed) {

This comment has been minimized.

@hadley

hadley Feb 19, 2016

Member

expectation_ok(exp)

@@ -0,0 +1,23 @@
context("Return values")

This comment has been minimized.

@hadley

hadley Feb 19, 2016

Member

I think this should probably go in test_test_that.R

@@ -12,3 +12,18 @@ expect_gte(3, 3)
expect_less_than(2, 3)
expect_lt(2, 3)
expect_lte(2, 2)
expect_error(expect_false(FALSE), NA)

This comment has been minimized.

@hadley

hadley Feb 19, 2016

Member

I think you accidentally re-added this?

This comment has been minimized.

@krlmlr

krlmlr Feb 19, 2016

Member

This was intentional, the commented code that was below is now gone.

Kirill Müller added some commits Feb 19, 2016

Kirill Müller
Kirill Müller
@krlmlr

This comment has been minimized.

Member

krlmlr commented Feb 19, 2016

Done. One more thing: expectation() is exported, you have changed its interface. Perhaps it's safer to keep success_msg (with a warning), or introduce a ... .

@hadley

This comment has been minimized.

Member

hadley commented Feb 19, 2016

Yeah, I'm not too worried about that. I think the next release will be testthat 1.0.0 so it's ok to make a few breaking changes (and I doubt this one will affect many people)

@hadley

This comment has been minimized.

Member

hadley commented Feb 19, 2016

And looks good to merge - feel free whenever you think it's ready

krlmlr added a commit that referenced this pull request Feb 19, 2016

Merge pull request #360 from krlmlr/feature/return-value
* Refactored internal testing logic. Now all expectations are also conditions, and R's condition system is used to signal failures and successes (#360, @krlmlr).

* `test_that()` returns a `logical` that indicates if all tests were successful (#360, @krlmlr).

@krlmlr krlmlr merged commit 0719724 into r-lib:master Feb 19, 2016

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@krlmlr krlmlr deleted the krlmlr:feature/return-value branch Feb 19, 2016

@krlmlr krlmlr referenced this pull request May 7, 2016

Merged

New DebugReporter #470

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment