Skip to content

Only respond to new check-expects in REPL - #230

Merged
mfelleisen merged 1 commit into
racket:masterfrom
shhyou:clear-tests-in-repl
Oct 26, 2024
Merged

Only respond to new check-expects in REPL#230
mfelleisen merged 1 commit into
racket:masterfrom
shhyou:clear-tests-in-repl

Conversation

@shhyou

@shhyou shhyou commented Oct 26, 2024

Copy link
Copy Markdown
Collaborator

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

  • #lang SLs: all test results go to REPL
  • menu-based SLs: reports go to a separate window

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.
...

@mfelleisen
mfelleisen merged commit 15df8c0 into racket:master Oct 26, 2024
@rfindler

Copy link
Copy Markdown
Member

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

mfelleisen commented Oct 26, 2024 via email

Copy link
Copy Markdown
Contributor

shhyou added a commit to shhyou/drracket that referenced this pull request Oct 26, 2024
shhyou added a commit to shhyou/drracket that referenced this pull request Oct 27, 2024
@rfindler

Copy link
Copy Markdown
Member

"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

shhyou commented Oct 27, 2024

Copy link
Copy Markdown
Collaborator Author

I added some tests to tests/drracket/module-lang-test in racket/drracket#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
shhyou deleted the clear-tests-in-repl branch October 27, 2024 15:58
@shhyou

shhyou commented Oct 27, 2024

Copy link
Copy Markdown
Collaborator Author

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

Copy link
Copy Markdown
Member

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).

shhyou added a commit to racket/drracket that referenced this pull request Nov 11, 2024
 (#689)

* Add some tests for racket/htdp#228, racket/htdp#229, and racket/htdp#230 to
  `tests/drracket/module-lang-test`.

  + Tests of racket/htdp#229 should first save the buffer to disk before running
  + Some racket/htdp#229 tests are ported from htdp-test:intm-lam.rktl

* Update `tests/drracket/language-test` to match racket/htdp#229.

* Manually check for empty stderr in tests/drracket/module-lang-test

  Somehow `-e`/`--check-stderr` does not work with `tests/drracket/module-lang-test`
  (perhaps due to using `test-log` + `exit 0` in `fire-up-drracket-and-run-tests`?)

* Test utils: sleep for 0.1s after printing error msg

  In tests/drracket/module-lang-test, stderr goes through a pipe to be
  checked for the absence of error messages. Therefore, sleep for 0.1s
  before existing to let the background thread pipe the messages to terminal.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants