pbench-results-push: Backport of #3348 and #3378 to b0.72 #3377
Conversation
There was a problem hiding this comment.
I have some concerns. I don't know whether these issues were present in the main PR, but perhaps I just missed them. (I do remember commenting on the obsolete comment lines....)
Looking at test_results_push.py, I believe that there are now two redundant tests which should be removed (there might be others that I didn't catch...), and I think the default response status code is broken (which leaves me wondering why the tests are passing), which brought to light the fact that a few of the tests shouldn't be setting up responses at all.
If these issues are present in main, they should probably be fixed there, first, and then this PR should be updated. However, if this PR is just bringing b0.72 up to the currently questionable state of main, then I suppose it's OK.
So, I'm neither approving nor blocking this merge...I'll leave it up to you.
ndokos
force-pushed
the
pbench-results-push-backport-0.72
branch
from
6a05d61 to
daa6e63
Compare
ndokos
changed the title
…istributed-system-analysis#3378 to b0.72 Cherry-pick the two commits from the above PRs into b0.72. No changes were necessary and no conflicts arose. * Do not use the file logger at all in python commands. The logger only uses the default Streamhandler now which outputs to stderr. * - copy_result_tb: return the response to the PUT request copy_result_tb returns the response from the server. The callers are responsible for interpreting it. However, it can still raise an exception (e.g. on connection error). - push.py: construct a reasonable message out of the response and check if the HTTP status is 201: we exit with 0 if so. Otherwise, if the status is OK (i.e. < 400), then we exit with 0 but print a message with a reasonably self-explanatory status on stderr. We expect that we will only ever get two OK responses: a 201 which indicates a successful PUT with a newly created dataset and 200 which indicates a duplicate dataset. In all non-OK cases, we output the message on stderr and exit with 1. - move.py: check the response - if not OK (>= 400) raise exception. * Fix the monkeypatching in test_copy_result_tb.py Monkeypatching pathlib components (.exists() and .open()) causes pytest to blow up. Limit the scope by using `monkeypatch.context()' so that it is only applied to the objects under test (and not e.g. to what is used in checking assertions). * Parametrize the "normal" test in test_push_results.py Add the connection error test to the parametrized set and eliminate some (now) redundant tests.
test_multiple_meta_args() should not have been deleted.
ndokos
force-pushed
the
pbench-results-push-backport-0.72
branch
from
f43e073 to
2d44db7
Compare
Cherry-pick the two commits from the above PRs into b0.72.
No changes were necessary and no conflicts arose.
The logger only uses the default
Streamhandlernow which outputs tostderr.copy_result_tb: return the response to thePUTrequestcopy_result_tbreturns the response from the server. The callers are responsible for interpreting it. However, it can still raise an exception (e.g. on connection error).push.py: construct a reasonable message out of the response and checkif the HTTP status is
201: we exit with0if so.Otherwise, if the status is OK (i.e. < 400), then we exit with
0but print a message with a reasonably self-explanatory status onstderr. We expect that we will only ever get two OK responses: a201which indicates a successfulPUTwith a newly created dataset and200which indicates a duplicate dataset.In all non-OK cases, we output the message on
stderrand exit with1.move.py: check the response - if not OK (>= 400) raise exception.test_copy_result_tb.pyMonkeypatching
pathlibcomponents (.exists()and.open()) causes PyTest to blow up. Limit the scope by usingmonkeypatch.context()so that it is only applied to the objects under test (and not e.g. to what is used in checking assertions).Add the connection error test to the parametrized set and eliminate some (now) redundant tests.