Error not recorded if it happens in expect_message()

[krlmlr]

Adapted reporter test, currently failing.

Closes #387 (=includes it).

[krlmlr]

The error is recorded, it's just not printed, because it's handled when sink() is still active. This also affects expect_warning() and expect_output(). I've added the resulting differences to this PR. Please advise.

[krlmlr]

Seems tricky. Best idea so far: Use a new CachingReporter in evaluate_promise() that defers actual reporting until the sink is closed again. Will try.

[krlmlr]

Works for me now, PTAL. I'm not inheriting from SilentReporter because I need to forward context and test, too.

[hadley]

You'll also need to tweak the constants in test_code() to avoid the extra stuff here

[krlmlr]

Done, requires #387 (already included in here).

[krlmlr]

Depends on #387 (which needs #390); let's wait for these two.

[hadley]

Can you please rebase/merge and I'll give a detailed review

[krlmlr]

Rebased. I wonder if we can do anything about this srcref loss here.

[hadley]

I think I need a bit more info on this problem. Your test:

reporter <- with_reporter("silent", {
  test_that("", expect_warning(stop()))
})
expect_equal(length(reporter$expectations()), 1)
expect_true(expectation_error(reporter$expectations()[[1L]]))

Currently works for me, and that makes sense - the error causes a premature exit from evaluate_promise(), so by the time the reporter prints something the sink has been terminated.

It's not clear your patch preserves the desirable behaviour of suppressing warnings when called from expect_warning(), while propagating them when called from expect_message(). I think what we actually need is finer control over evaluate promise so we can opt out of capturing messages or warnings. I'll tweak evaluate_promise() to do this.

Can you have a go at creating a simple test case that illustrates the current problem?

[krlmlr]

Summary of commits:

• fe2ea0e is the demo
• 133c75e is the fix, which solves the issue (with minor flaws)
• 27536b7 is the correction of the call stack, which (almost) restores the original behavior.

Could you sketch an example where this implementation would misbehave?

[krlmlr]

I don't like this anymore either: My new DebugReporter (internal WIP) doesn't work here. Perhaps we can teach evaluate_promise() to temporarily close its sink when a reporter is activated.

[hadley]

I think the way forward is to break evaluate_promise() into 3 separate pieces, one that captures outputs, one that captures warnings, and one that captures messages.