racket / htdp

Other
93 stars 70 forks source link

Only respond to new `check-expect`s in REPL #230

Closed shhyou closed 1 month ago

shhyou commented 1 month ago

lang SLs and menu-based SLs use different test engine UIs:

Consequently, when a new check-expect is executed from REPL, I think #lang SLs should only respond to it rather than printing the outcome of all existing tests. In contrast, it makes sense for menu-based SLs to retain its current behavior.

This PR changes #lang SLs by clearing all tests before executing REPL inputs. It does not affect menu-based SLs.

Example program:

#lang htdp/bsl
(check-expect (+ 2 3) 5)
(check-expect (+ 2 3) 6)

Old behavior:

Welcome to DrRacket!
Ran 2 tests.
1 of the 2 tests failed.

> (check-expect (* 2 3) 6)
Ran 3 tests.
1 of the 3 tests failed.
                              ;;        no good
Check failures:               ;;      ↙
        Actual value 5 differs from 6, the expected value.
at line 3, column 0

> (check-expect (error 'oops) 123)
Ran 4 tests.
2 of the 4 tests failed.
...

New behavior:

#lang htdp/bsl
(check-expect (+ 2 3) 5)
(check-expect (+ 2 3) 6)

Welcome to DrRacket!
Ran 2 tests.
1 of the 2 tests failed.

> (check-expect (* 2 3) 6)
The test passed!

> (check-expect (error 'oops) 123)
Ran 1 test.
0 tests passed.
...
rfindler commented 1 month ago

This seems like an improvement to me too, but I wonder if it would be easy to make the new behavior like this:

#lang htdp/bsl
(check-expect (+ 2 3) 5)
(check-expect (+ 2 3) 6)

Welcome to DrRacket!
Ran 2 tests.
1 of the 2 tests failed.

> (check-expect (* 2 3) 6)
The test passed!

> (check-expect (error 'oops) 123)
The test failed.

Also, I think it is possible to add a test case for this. Is there already a test suite for repl stuff in the #lang teaching languages?

mfelleisen commented 1 month ago

"Also, I think it is possible to add a test case for this. Is there already a test suite for repl stuff in the #lang teaching languages?”

Really? I am unaware of this test suite. Is it in the drracket world? (If I had known I would have pointed Shuhung there.)

rfindler commented 1 month ago

"Also, I think it is possible to add a test case for this. Is there already a test suite for repl stuff in the #lang teaching languages?” Really? I am unaware of this test suite. Is it in the drracket world? (If I had known I would have pointed Shuhung there.)

I think that the module-lang test suite in the DrRacket world is close enough that it could be made to fit, but looking at the changes I think that probably it is preferable to do something inside just this repo. One could use module->namespace and then eval to simulate the REPL and it may be useful to have multiple such tests that are outside DrRacket, as I think that Emacs mode and just using racket at the command-line can get folks a REPL inside a teaching language program that's written in the #lang-based teaching languages.

shhyou commented 1 month ago

I added some tests to tests/drracket/module-lang-test in https://github.com/racket/drracket/pull/689 along with the tests for the two other PRs. (I think it's not urgent to merge them.)

@rfindler wrote: Also, I think it is possible to add a test case for this. Is there already a test suite for repl stuff in the #lang teaching languages?

Yes, but the setup of tests/drracket/test-engine-test is dedicated to menu-based SLs -- at least none of them failed with this PR. I've been using tests/drracket/module-lang-test which works for all #langs.

shhyou commented 1 month ago

This seems like an improvement to me too, but I wonder if it would be easy to make the new behavior like this:

> (check-expect (error 'oops) 123)
The test failed.

How about I open an issue about this? I think it's simpler if the response (i.e. "the test failed") is not just for the REPL but for the whole test engine.

rfindler commented 3 weeks ago

This seems like an improvement to me too, but I wonder if it would be easy to make the new behavior like this:

> (check-expect (error 'oops) 123)
The test failed.

How about I open an issue about this? I think it's simpler if the response (i.e. "the test failed") is not just for the REPL but for the whole test engine.

Okay.

Do you want to merge this one now? It seems okay to me to do so (and we can merge the tests in the other PR in drr when you're ready over there).