Fix incorrect usage of optional message in rackunit-test

[sorawee]

The tests either meant to use check = without an epsilon, or check-= with an epsilon. Currently, it uses check = with an epsilon, so the epsilon becomes a message. This PR fixes the tests.

Furthermore, there should be a guard to check that the optional message argument is indeed either #f or string?. This PR also does that.

[samth]

As @bogdanp noticed, this breaks one web-server test because the optional message argument was something else. We should probably just support that and convert to a string.

[sorawee]

The web-server test is objectively incorrect. It was wrongly assumed that the last argument is a regex pattern to be matched against the exception (so the fix is not just converting the regex to string, but also needing to restructure the entire test). So are the tests being fixed in this PR. It was wrongly assumed that the last argument is the epsilon (so either the epsilon must be dropped, or it must be refactored to use the epsilon somehow)

So while we can perform the conversion, that would silently hide bugs.

This is not just an undefined behavior that is undesirable. The documentation has always been clear that it's supposed to be a string. So I don't think that we should relax it.

[mfelleisen]

I had to fix a test in realm because of the stricter enforcement, and I think it is usually a problem with the test. This backwards incompatibility seems okay.

[samth]

On the one hand, I probably agree with @sorawee's argument in the abstract. On the other hand, these packages now have failing tests that didn't before:

Test Failures

• [x] crc32c https://plt.cs.northwestern.edu/pkg-build/server/built/test-fail/crc32c.txt
• [x] geoid https://plt.cs.northwestern.edu/pkg-build/server/built/test-fail/geoid.txt
• [ ] gnucash https://plt.cs.northwestern.edu/pkg-build/server/built/test-fail/gnucash.txt
• [ ] gregor-test https://plt.cs.northwestern.edu/pkg-build/server/built/test-fail/gregor-test.txt
• [ ] multiscope https://plt.cs.northwestern.edu/pkg-build/server/built/test-fail/multiscope.txt
• [x] r16 https://plt.cs.northwestern.edu/pkg-build/server/built/test-fail/r16.txt
• [ ] ~racquel https://plt.cs.northwestern.edu/pkg-build/server/built/test-fail/racquel.txt~
• [x] satore https://plt.cs.northwestern.edu/pkg-build/server/built/test-fail/satore.txt
• [x] seq https://plt.cs.northwestern.edu/pkg-build/server/built/test-fail/seq.txt
• [x] sudoku-solver https://plt.cs.northwestern.edu/pkg-build/server/built/test-fail/sudoku-solver.txt
• [ ] suffixtree https://plt.cs.northwestern.edu/pkg-build/server/built/test-fail/suffixtree.txt
• [ ] unlike-assets-test https://plt.cs.northwestern.edu/pkg-build/server/built/test-fail/unlike-assets-test.txt
• [x] with-cache https://plt.cs.northwestern.edu/pkg-build/server/built/test-fail/with-cache.txt

[sorawee]

Oh no. Thanks for tracking these issues.

• crc32c is harmless. As far as I can tell, it intentionally uses the object as a a message

• geoid does the following: (check-pred < start end). It really must be fixed

• gnucash looks like a copy-paste mistake? (introduced in https://github.com/jbclements/gnucash/commit/6dbe1eac10815ac7cf9794379a133100b9a0fd1d)

• gregor-test: a copy-paste mistake.

  (check-equal? (-minutes (datetime 2000 2 29) 1) (datetime 2000 2 28 23 59) 1)

  where the "message" 1 is copied from 1 on the left.

• multiscope: a copy-paste mistake from the previous test.

  (check-equal? (eval 'x) 5)
  
  (check-exn
    (lambda (e)
      (string-contains? (exn-message e) "s1: undefined"))
    (lambda ()
      (eval '(s1 x))) 5)

• r16: definitely not meant to be a message there

   (check-false (update-trick! db context-id "nonexistent-id" values (const #t))
                (check-false (remove-trick! db context-id "nonexistent-id" (const #t))))

• satore is harmless. As far as I can tell, it intentionally uses the object as a a message.

• seq: looks like a a copy-paste mistake from an earlier test

     (check-equal? (remove "banana" (set "apple" "banana" "cherry")) (set "apple" "cherry"))
     ...
     (check-exn exn:fail:contract?
                (thunk (drop-when (curry = "banana") (set "apple" "banana" "cherry"))) (set "apple" "cherry"))

• sudoku-solver: incorrect understanding of how check-true and check-false work. It was assumed that (check-true A B) checks that both A and B are true.

    (test-case
      "`row-valid?` and `index-valid?`"
      (define test-board (list->board test-board-data))
      ; Indices 12 and 16 are in the second row (1-based).
      (check-true (row-valid? test-board 12)
                  (index-valid? test-board 12))
      (define new-board (board-set test-board 16 9))
      (check-false (row-valid? new-board 12)
                   (index-valid? new-board 12)))

  This really must be fixed.

• suffixtree: definitely not meant to be a message there:

       (make-test-case
        "label-source-eq?"
        (let* ((label-1 (string->label "jump"))
               (label-2 (string->label "jump")))
          (check-true (label-source-eq? (sublabel label-1 0 2)
                                         (sublabel label-1 2))
                       (check-false (label-source-eq? label-1 label-2)))))

• unlike-assets-test: I'm impressed that it even managed to build, since the "source checksum" was incorrectly entered. Anyhow, it assumes that (check-true A B) checks that both A and B are true, and really must be fixed.

    (check-true (hash-has-key? (sys) "a")
                (live-build? (hash-ref (sys) "a"))))

• with-cache: probably a copy-paste mistake from an earlier test

  (check-equal? ((read/keys id good-keys equal?) v0) x)
  
  (check-false ((read/keys id bad-keys equal?) v0) x))

[mfelleisen]

On Jun 28, 2022, at 2:01 AM, sorawee @.***> wrote:

As far as I can tell, it intentionally uses the object as a a message

Ask the owners to use (~a (object-name o)) or perhaps advertise the idea, because it shows up in two repos. (I have done this too.)

[sorawee]

Thanks! I submitted PRs to address all of these issues except unlike-assets-test.

The issue with unlike-assets-test is that the version on the package server is a git commit (which in fact doesn't even exist...), so it is not possible to "fix" anything. As far as I can see, the latest commit of unlike-assets switches to use Denxi, so unlike-assets-test no longer exists, and that particular test was also removed at some point.

CC: @zyrolasting

[zyrolasting]

@sorawee Thanks for pinging me on this. Yes, that's the history, since I'm slowly (and painfully) migrating my code. I'm a little tight on time, but I will update the package source to something less vulnerable to my movements.