Stepper breaks test-engine contract

mikesperber commented 8 months ago

Run this in the stepper with the current head of racket/htdp:

#lang htdp/bsl
(check-expect 1 1)

You get

add-test!: expects a boolean, given (cons #true '())
  in: the range of
      the 1st argument of
      (-> (-> boolean?) any)
  contract from: 
      <pkgs>/htdp-lib/test-engine/test-engine.rkt
  blaming: <pkgs>/htdp-lib/test-engine/syntax.rkt
   (assuming the contract is correct)
  at: <pkgs>/htdp-lib/test-engine/test-engine.rkt:18:11

(This also shows up in the stepper tests.)

So somehow the return value from the check-expect test (which is supposed to be just plain #true) gets wrapped in a one-element list. I banged by head against this for a while, but am getting nowhere, unfortunately.

This is not a new problem - it's just popped up again as the contract for add-test! was tightened.

rfindler commented 8 months ago

I think that this line looks suspicious. Probably the (#%plain-app values results) should really be (#%plain-app apply values results).

When I make that change it seems to solve the original issue. @jbclements does that seem like the right fix to you?

jbclements commented 8 months ago

Hi there! Sitting in the airport in Amsterdam right now.

I just did 20 minutes of code spelunking, and it looks to me like I added this in 2008, in a commit that is now named f69ec2a6e47006272e9ebb7 … what I haven’t yet figured out is how it ever worked… it looks like the result of the code will always always be putting this in a list. I guess maybe that result isn’t used, but it’s both appalling to me that I didn’t find this sooner, and worrying to me that something else might now inadvertently depend on this. A comment (from 2008) suggests that the use-val-as-final tag is intended for use in test cases. I guess my next step is to take a look at where it’s used. Mike, I think you now know way more about the testing framework than I do, and you mention that this is not a new problem, it’s just resurfaced. Can you … tell me a bit more about that?

Long story short, I think Robby’s solution is almost certainly correct, so many thanks for finding this!

John

On Dec 29, 2023, at 03:42, Robby Findler @.***> wrote:

I think that this line https://github.com/racket/htdp/blob/master/htdp-lib/stepper/private/annotate.rkt#L669 looks suspicious. Probably the (#%plain-app values results) should really be (#%plain-app apply values results).

When I make that change it seems to solve the original issue. @jbclements https://github.com/jbclements does that seem like the right fix to you?

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

mikesperber commented 8 months ago

@jbclements In the past, nothing used the return value of the tests, so that's why it was OK to ignore, and also why there was this lame any in the relevant contract. (I should really have reported this back then when I noticed it first but was too unsure of how things are supposed to work with the stepper - sorry!) This just changed, so the contract was tightened.

racket / htdp

Stepper breaks test-engine contract #214