Make OpenAPI/Swagger tests independent of parameter array order - #798
Conversation
| (testing "spec is valid" | ||
| (is (nil? (validate spec)))))))) | ||
|
|
||
| (deftest parameter-location-order-independent-of-map-seq |
There was a problem hiding this comment.
overkill, maybe?
| (is (= ["query" "body" "formData" "header" "path"] | ||
| (map :in (get-in spec [:paths "/parameters" :post :parameters])))))) | ||
|
|
||
| (deftest parameter-location-order-independent-of-map-seq |
e1fc344 to
39f83e2
Compare
| strip-endpoint-keys #(dissoc % :id :parameters :responses :summary :description) | ||
| openapi (->> (strip-endpoint-keys openapi) | ||
| (merge {:openapi "3.2.0" | ||
| (merge {:openapi "3.1.0" |
There was a problem hiding this comment.
Merge/rebase artifact likely, openapi upgrade shouldn't be reverted.
There was a problem hiding this comment.
good catch! fixed!
|
Quick comment: I'm not sure I like this hack. I feel that the ordering should get fixed at json-serialisation-time, not in the clojure data structures. |
3029278 to
3bdb298
Compare
The-Alchemist
changed the title
I think you're totally right; I fixed the tests to be sort-order independent. let me know what you think! |
opqdonut
left a comment
opqdonut
left a comment
There was a problem hiding this comment.
The approach is good. I have some improvement suggestions on small details, but I can also implement those myself if you don't feel like making yet another revision of this. Let me know which way you want to do it!
| :required true | ||
| :description "description :p" | ||
| :schema {:type "string"}}] | ||
| (is (match? (sorted-parameters |
There was a problem hiding this comment.
bw matcher-combinators has in-any-order, which could take care of this
| :required true | ||
| :type "string"}]) | ||
| (sorted-parameters | ||
| (normalize |
There was a problem hiding this comment.
sorted-parameters could be folded into normalize in this test namespace
| spec (:body (app {:request-method :get, :uri "/swagger.json"})) | ||
| ins (map :in (get-in spec [:paths "/parameters" :post :parameters]))] | ||
| (is (= 5 (count ins))) | ||
| (is (= #{"query" "body" "formData" "header" "path"} (set ins))))) |
There was a problem hiding this comment.
btw, you could get by with one is by using (is (= [...] (sort ...)))
| methods | ||
| methods))) | ||
| paths | ||
| paths))))) |
There was a problem hiding this comment.
I'm tempted to code-golf this, but I can do it after merging this PR 😁
Clojure maps do not guarantee seq order, so `=` on :parameters vectors is JVM-dependent. Parameter order is not meaningful in the spec; sort by :in and name in assertions instead of canonicalizing production output. Signed-off-by: The-Alchemist <kap4020@gmail.com>
Signed-off-by: The-Alchemist <kap4020@gmail.com>
3c77bda to
ab13435
Compare
3 participants
Summary
:parametersare unordered arrays. Tests previously used=/match?on those vectors, which is JVM-dependent because Clojure maps do not guarantee seq order when the spec is built from:parametersmaps.:parametersin the tests by:inand name before comparing. Production emission is unchanged.Motivation
Clojure's map contract does not promise insertion order except for
array-map. Alternate JVM implementations (like our Cloffle, based on Clojure on GraalVM) can emitpathbeforequery. Spec consumers and golden tests compare parameter vectors, which are order-sensitive.We currently maintain a separate patch to the tests because we run
reititas part of our compatibility suite.Compatibility
No public API change and no change to generated OpenAPI/Swagger output. Only test helpers and assertions were updated.