Treat exceptions thrown by arguments to check as test failures (again)

[sorawee]

This PR corrects the wrong fix in #123. It restores the functionality that #109 is meant to implement while correctly reporting test results.

CC: @AlexKnauth and @wilbowma

[AlexKnauth]

It's a shame that conditional is necessary... but yes, this looks like the right thing to do to correct my mistake in #109

[sorawee]

Please don't merge this PR yet. I just discovered several issues when running the test suite. Weirdly, the CI and my local testing last week didn't find any issue...

[AlexKnauth]

What are the issues?

[sorawee]

Last week when I run raco test rackunit-test there's an error in rackunit-test/tests/rackunit/check-info-test.rkt because the fields are not ordered correctly, but I cannot reproduce it anymore... I don't know what's going on.

Anyway, I just pushed a code to avoid the conditional expression. If the CI returns green, I think it can be merged.

[samth]

We really need to come to a conclusion here. @sorawee @AlexKnauth is this ready to merge?

[AlexKnauth]

This looks good to me

[samth]

@racket/release I think this should go in 8.1.

[samth]

This again caused test failures. http://drdr.racket-lang.org/57098/cs/racket/share/pkgs/rackunit-test/tests/rackunit/check-info-test.rkt http://drdr.racket-lang.org/57098/cs/racket/share/pkgs/rackunit-test/tests/rackunit/standalone.rkt

What's going on here? Why didn't this show up in CI?

[sorawee]

Let's just revert it for now. I don't want it to interfere the 8.1 release.

The issue with CI seems to be that it's not running the current code / current test. I added a test named 109+138.rkt, but that doesn't seem to get run by the CI.

What's more weird is why I couldn't reproduce the issue locally three days ago though now I can...

[samth]

Well, we're past the branch so this isn't interfering with the release.

Why didn't the CI discover the errors that DrDr found? One possibility is that the check-info-test is dependent on hash table ordering, but it also seems to print to stderr, which maybe wasn't noticed by the GHA CI.

[sorawee]

Just saw your message. Are you OK with letting it failing in DrDr?

In any case, I can investigate the issue later this week.

[sorawee]

Why didn't the CI discover the errors that DrDr found? One possibility is that the check-info-test is dependent on hash table ordering, but it also seems to print to stderr, which maybe wasn't noticed by the GHA CI.

It can detect stderr just fine. See the CI error in #139, which is the current error in DrDr.

So I'm even more confident now that the CI downloads the current code from the package server, so it doesn't actually test the change that is being submitted.

[samth]

Yes, that's definitely happening, although I'm not clear on why.

[samth]

Any further thoughts on the problem here?

[sorawee]

Sorry, I meant to take a look at this and forgot. Will do that in this weekend.

[samth]

Ok, I just spent some time looking at this and the problem is pretty fundamental. The issue is that the check-info for a failure during the argument evaluation doesn't have the params info. That check was added by @sorawee in 92b6812e6e6b27e74ab19728447cf4e279477eba. However, that's fundamentally impossible -- the params info is supposed to have the values of the parameters which it can't because we're in the process of evaluating them. The test only passed because the code was doing the wrong thing before this patch (because of #123).

So we have to pick some other behavior. On possibility is the name of the parameter. Another possibility is just blessing the current behavior (although that has a weird consequence in the check-info ordering that causes the other test to fail). Another possibility is #f or something else placeholder-like.

[samth]

141 ought to work, but doesn't, for reasons I don't understand.