greghendershott / aws

Racket support for Amazon Web Services.
BSD 2-Clause "Simplified" License
78 stars 25 forks source link

sigv4 issues #64

Closed jeapostrophe closed 4 years ago

jeapostrophe commented 5 years ago

Any idea what is going on in this test failure?

http://drdr.racket-lang.org/53149/racket/share/pkgs/aws/aws/sigv4.rkt

greghendershott commented 5 years ago

Not yet. Looking now. The failure doesn't occur in Racket versions 6.2 through 7.3, just HEAD.

First quick guess is HEAD changed something about hash-table ordering (an assumption easy to bake incorrectly into tests) or macro expansion (the test uses something defined as a macro instead of a function, for reasons that don't make sense to me now).

Trying a couple quick experiments to maybe narrow it down. Will report back.

greghendershott commented 5 years ago

@jeapostrophe I narrowed it down. Apparently the behavior of in-dict changed with respect to association lists containing duplicate "keys":

;; in-dict-changed.rkt
#lang racket
(require rackunit)
(displayln (banner))
(check-equal? (for/list ([(k v) (in-dict '((a . "1") (a . "2")))])
                (cons k v))
              '((a . "1") (a . "2")))
greg@x1c:~$ ~/racket/7.3/bin/racket /tmp/in-dict-changed.rkt 
Welcome to Racket v7.3.

greg@x1c:~$ ~/racket/7.5.0.6/bin/racket /tmp/in-dict-changed.rkt 
Welcome to Racket v7.5.0.6.

--------------------
FAILURE
name:       check-equal?
location:   /tmp/in-dict-changed.rkt:4:0
actual:     '((a . "1"))
expected:   '((a . "1") (a . "2"))
--------------------

I don't know if that was intentional but it seems like a breaking change -- and one which could break more programs than mine?

samth commented 5 years ago

This change was in https://github.com/racket/racket/pull/2846 which was intentional but I think not intended to break things.

greghendershott commented 5 years ago

Ha I had just found that commit myself and was about to comment.

Yes it looks like that commit fixes other problems, but introduces this one.

Hopefully there is a way to push down the bump in the carpet without it popping up somewhere else.

jbclements commented 4 years ago

I'm confirming that the test above does not fail on the release candidate, 7.4.902, so I'm removing this item from the release checklist.