pbench-results-push: logging fixes and better reporting

[ndokos]

• Disable file logging from all agent python commands. Also clean up some now unnecessary cruft.
• Changes copy_result-tb to return the server response without interpreting it (although it still raises exceptions on some errors, e.g. connection errors). The response status codes are interpreted in push.py and move.py. push.py is silent on success; it outputs an error message but still exits with 0 on any other OK status; otherwise, it outputs an error message and exits with 1. move.py translates all non-OK responses to exceptions.
• Limit the scope of monkeypatching of some pathlib' components that were used by pytest in checking assertions and were causing internal failures in pytest' itself.
• Parametrize some tests of pbench-results-push - there is more to be done here (see issue Testing fixes for test_results_push.py #3374).

[webbnh]

The Jenkins issue looked like a transient failure, so I re-started the build.

[webbnh]

It looks like we're not handling failure properly.

[webbnh]

And, some of the tests need additional assertions.

[ndokos]

I got rid of the changes to move.py (and its tests): although that will need attention, I just want to concentrate on push for this PR. With that proviso, I think I've addressed all the comments, except parametrizing the tests. I'll revisit that at some point but I don't want to do it now.

[dbutenhof]

I think we're reaching the point where it'd be nice to get it in; so while I'm quibbling about the testing, I'm going to approve anyway.

[siddardh-ra]

Looks good

[webbnh]

Looks good, I just have extensions to Dave's nits.

[webbnh]

I think move.py needs to be updated to check for and handle errors from copy_result_tb().

[ndokos]

I think this would work, no? I'll put it back in draft mode and go do the tests if that looks OK.

[webbnh]

There are a few rough edges here now which need smoothing.

[dbutenhof]

A couple of places this could be streamlined, removing some of your comments about "trouble" areas ... but I won't obsess about it.

[webbnh]

It seems straightforward enough:

        if status_code:
            if isinstance(message, dict):
                responses.add(
                    responses.PUT,
                    f"{TestResultsPush.URL}/upload/{os.path.basename(tarball)}",
                    status=status_code,
                    json=message,
                )
            elif isinstance(message, (str, BufferedReader, Exception)):
                responses.add(
                    responses.PUT,
                    f"{TestResultsPush.URL}/upload/{os.path.basename(tarball)}",
                    status=status_code,
                    body=message,
                )
            else:
                responses.add(
                    responses.PUT,
                    f"{TestResultsPush.URL}/upload/{os.path.basename(tarball)}",
                    status=status_code,
                )
        else:
            responses.add(
                responses.PUT,
                f"{TestResultsPush.URL}/upload/{os.path.basename(tarball)}",
            )

Or, my biases lead me to try a single call with conditional data:

        parms = {}
        if status_code:
            parms["status"] = status_code

        if isinstance(message, dict):
            parms["json"] = message
        elif isinstance(message, (str, BufferedReader, Exception)):
            parms["body"] = message

        responses.add(
            responses.PUT,
            f"{TestResultsPush.URL}/upload/{os.path.basename(tarball)}",
            **parms,
        )

[ndokos]

Yes, that's basically what I did (the first version) but where did the BufferedReader and Exception cases come from? I don't understand why they are needed.

I'll do one more push using your single call example probably.

[webbnh]

Those are the types, according to the documentation, that the body parameter will accept.

[webbnh]

Looks good. There are just two (new) small things which we can ignore if we must.

I leave it up to you whether to add Exception back to the list of types which add_http_mock_response() can handle (but, if you add it, then we can use add_http_mock_response() in place of the existing add_connectionerr_mock_response()...).

[webbnh]

Since the value of message is determined by the parametrize() list which we control, having a try block here seems overdone.

[ndokos]

I opened issue #3374 to deal with the remaining problems and objections. I think I have captured them all.

[webbnh]

I'll grant that the numeric values of the HTTPStatus codes are all well-defined, but, for the most part, they are not ordered, so applying a relational operator (>=) to them seem inappropriate.

Choosing the least of evils, here, I would use a bare 400, as that is an architected number (although, it would be better if we had a symbolic constant for it with an appropriate semantic, unlike BAD_REQUEST).

[ndokos]

I opened issue #3374 to deal with the remaining problems and objections. I think I have captured them all.