apiv2 tests: finally fix POST as originally intended #9686
Conversation
When I originally wrote this code I had no idea what POST
would look like so I did a sloppy job, deferring making it
usable. Now that we have some real-world examples in place,
I have a better understanding of what params look like and
how to make tests more readable/maintainable. (Deferring isn't
always bad: one of my early ideas was to separate params using
commas; that would've been a disaster because some JSON values,
such as arrays, include commas).
This commit implements a better way of dealing with POST:
* The main concept is still 'key=value'
* When value is a JSON object (dictionary, array), it
can be quoted.
* Multiple params are simply separated by spaces.
The 3-digit HTTP code is a prominent, readable separator
between POST params and expected results. The parsing
code is a little uglier, but test developers need
never see that. The important thing is that writing
tests is now easier.
* POST params can be empty (this removes the need for a
useless '')
I snuck in one unrelated change: one of the newly-added
tests, .NetworkSettings, was failing when run rootless
(which is how I test on my setup). I made it conditional.
Signed-off-by: Ed Santiago <[email protected]>
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: edsantiago The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci-robot
added
the
approved
|
Review Suggestions: I realize this is a mess to review. If I may make a couple of suggestions:
|
| # Test compat API for Network Settings | ||
| # Test compat API for Network Settings (.Network is N/A when rootless) | ||
| network_expect= | ||
| if root; then | ||
| network_expect='.[0].NetworkSettings.Networks.podman.NetworkID=podman' | ||
| fi | ||
| t GET /containers/json?all=true 200 \ | ||
| length=1 \ | ||
| .[0].Id~[0-9a-f]\\{64\\} \ | ||
| .[0].Image=$IMAGE \ | ||
| .[0].NetworkSettings.Networks.podman.NetworkID=podman | ||
| $network_expect |
There was a problem hiding this comment.
This is the one non-param-related change, the one I snuck in. All it does is, if running rootless, skip the .NetworkSettings check but run the others.
| t POST myentrypoint 200 ! no params | ||
| t POST myentrypoint id=$id 200 ! just one | ||
| t POST myentrypoint id=$id filter='{"foo":"bar"}' 200 ! two, with json | ||
| t POST myentrypoint name=$name badparam='["foo","bar"]' 500 ! etc... |
There was a problem hiding this comment.
Would foo="value with space" be counted as one argument?
There was a problem hiding this comment.
Oh, good question. I was careful to do arg processing the "correct" way, so yes, it should work, but I don't have any explicit tests for it. I think it might be time to add an internal test suite for that helper. Unfortunately, my sneak-in-some-work-time for today has run out! I'll get to that when I can. Thanks for pointing it out.
|
/lgtm |
openshift-ci-robot
added
the
do-not-merge/hold
|
/hold cancel |
openshift-ci-robot
removed
the
do-not-merge/hold
github-actions
bot
added
the
locked - please file new issue/PR
When I originally wrote this code I had no idea what POST
would look like so I did a sloppy job, deferring making it
usable. Now that we have some real-world examples in place,
I have a better understanding of what params look like and
how to make tests more readable/maintainable. (Deferring isn't
always bad: one of my early ideas was to separate params using
commas; that would've been a disaster because some JSON values,
such as arrays, include commas).
This commit implements a better way of dealing with POST:
can be quoted.
The 3-digit HTTP code is a prominent, readable separator
between POST params and expected results. The parsing
code is a little uglier, but test developers need
never see that. The important thing is that writing
tests is now easier.
useless '')
I snuck in one unrelated change: one of the newly-added
tests, .NetworkSettings, was failing when run rootless
(which is how I test on my setup). I made it conditional.
Signed-off-by: Ed Santiago [email protected]