racket / htdp

Other
91 stars 69 forks source link

Tests reported as passing eroneously when a top-level error occurs #212

Closed dbp closed 8 months ago

dbp commented 9 months ago

Running this:

#lang htdp/isl+

(define (badfn x)
  (error "hello"))

(check-expect (badfn 1) 1)

(error "hello")

Produces:

Screenshot 2023-11-29 at 9 05 27 AM

In the non-#lang it's better; while the interactions window still prints the buggy "The test passed", a window pops up with the correctly failing test.

Interactions window:

Screenshot 2023-11-29 at 9 06 13 AM

Pop up:

Screenshot 2023-11-29 at 9 06 36 AM
samth commented 9 months ago

Which version is this? A similar bug #203 was fixed recently.

dbp commented 9 months ago

This is in 8.10; I don’t upgrade mid semester! (I trust you but not that much!). If it’s fixed in 8.11 sorry for the spurious report.

On Wed, Nov 29, 2023, at 9:16 AM, Sam Tobin-Hochstadt wrote:

Which version is this? A similar bug #203 https://github.com/racket/htdp/issues/203 was fixed recently.

— Reply to this email directly, view it on GitHub https://github.com/racket/htdp/issues/212#issuecomment-1831979884, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAELBJJ43M5P4N2YJKGRJQLYG47U5AVCNFSM6AAAAAA77Q4UEOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQMZRHE3TSOBYGQ. You are receiving this because you authored the thread.Message ID: @.***>

rfindler commented 9 months ago

I suspect it is fixed but, just in case it wasn't obvious and you want to try it out yourself, if you have your two version installations in separate directories, then you should be able to have both of them on your machine (this works especially well for me on mac os with multiple folders in /Applications plus a couple of git checkouts elsewhere for development, even).

mfelleisen commented 9 months ago

It is fixed. I double-checked.

shhyou commented 9 months ago

I'm on 8.11.0.3 [cs], racket/htdp@42f0782 (“stepper history no update for 8.11”) and racket/drracket@6257cb8 (“when receiving syntax objects from 'online-check-syntax logger”).

I still see the message "The test passed!" in #lang htdp/isl+ and Intermediate Student with lambda, although the test didn't pass. Am I missing anything?

shhyou commented 9 months ago

@dbp I didn't see the pop-up test engine window in your screenshot (could be due to version differences).

dbp commented 9 months ago

So, to confirm, this definitely seems like an issue in the released 8.11 (I see identical behavior, Apple Silicon build). And it seems like the #203 fixes were included in that?

mfelleisen commented 9 months ago

Oops .. I ran this in the menu based languages and didn't pay attention. So sorry.

rfindler commented 9 months ago

It looks like this example used to work, in version 8.9.

mikesperber commented 9 months ago

"Works" is relative, as two bugs cancelled each other out, and one of them got fixed with #203. I'm looking into this one.

mikesperber commented 9 months ago

@rfindler @dbp @mfelleisen @shhyou I see what's happening, but I need some guidance as to what the behavior should be / what the mental model here is supposed to be:

My mental model has been that the tests get run after the "regular" program has run, and that exceptions in the regular program stop execution.

In the program here, the "regular" program bombs - and the tests therefore don't get run. If that's basically what we want, it's just the message that's wrong, and I have a fix ready.

If we want the tests to run always - even if the main program fails - we need a different fix. (This was basically that 8.10 did, and I assumed that was wrong.)

rfindler commented 9 months ago

Your mental model matches what mine is.

Robby

On Sun, Dec 10, 2023 at 9:16 AM Mike Sperber @.***> wrote:

@rfindler https://github.com/rfindler @dbp https://github.com/dbp @mfelleisen https://github.com/mfelleisen @shhyou https://github.com/shhyou I see what's happening, but I need some guidance as to what the behavior should be / what the mental model here is supposed to be:

My mental model has been that the tests get run after the "regular" program has run, and that exceptions in the regular program stop execution.

In the program here, the "regular" program bombs - and the tests therefore don't get run. If that's basically what we want, it's just the message that's wrong, and I have a fix ready.

If we want the tests to run always - even if the main program fails - we need a different fix. (This was basically that 8.10 did, and I assumed that was wrong.)

— Reply to this email directly, view it on GitHub https://github.com/racket/htdp/issues/212#issuecomment-1848993410, or unsubscribe https://github.com/notifications/unsubscribe-auth/AADBNMDRMF64YE6AS3YGFSTYIXG6FAVCNFSM6AAAAAA77Q4UEOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQNBYHE4TGNBRGA . You are receiving this because you were mentioned.Message ID: @.***>

mfelleisen commented 9 months ago

Exactly.

On Dec 10, 2023, at 10:26 AM, Robby Findler @.***> wrote:

Your mental model matches what mine is.

Robby

On Sun, Dec 10, 2023 at 9:16 AM Mike Sperber @.***> wrote:

@rfindler https://github.com/rfindler @dbp https://github.com/dbp @mfelleisen https://github.com/mfelleisen @shhyou https://github.com/shhyou I see what's happening, but I need some guidance as to what the behavior should be / what the mental model here is supposed to be:

My mental model has been that the tests get run after the "regular" program has run, and that exceptions in the regular program stop execution.

In the program here, the "regular" program bombs - and the tests therefore don't get run. If that's basically what we want, it's just the message that's wrong, and I have a fix ready.

If we want the tests to run always - even if the main program fails - we need a different fix. (This was basically that 8.10 did, and I assumed that was wrong.)

— Reply to this email directly, view it on GitHub https://github.com/racket/htdp/issues/212#issuecomment-1848993410, or unsubscribe https://github.com/notifications/unsubscribe-auth/AADBNMDRMF64YE6AS3YGFSTYIXG6FAVCNFSM6AAAAAA77Q4UEOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQNBYHE4TGNBRGA . You are receiving this because you were mentioned.Message ID: @.***>

— Reply to this email directly, view it on GitHub https://github.com/racket/htdp/issues/212#issuecomment-1848995978, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAL7T6ZP2G33GYWWRAVZPJTYIXIDRAVCNFSM6AAAAAA77Q4UEOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQNBYHE4TKOJXHA. You are receiving this because you were mentioned.

mikesperber commented 9 months ago

Thanks - just pushed PR #213 to fix this.

rfindler commented 9 months ago

Matthias posted a follow up to the commit that closed this issue with this program:

(check-expect (g 30) "goodbye")
(f 40)
(define (f x) "hello")
(define (g y) "goodbye")

which, when I run it in the language-menu based version of BSL, I see:

Welcome to DrRacket, version 8.11.1.5--2023-12-11(0b2a5d97/d) [cs].
Language: Beginning Student; memory limit: 256 MB.
The test passed!f is used here before its definition
> 
shhyou commented 9 months ago

I just updated htdp-lib and see 0 tests passed.f is used here before its definition.

rfindler commented 9 months ago

Oh, I see that too now. I wonder if I used the wrong DrRacket in my tests (as I have updated only one of them so far).