Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix various problems in the pbench_results_push tests #3378

Merged
merged 3 commits into from
Apr 12, 2023

Conversation

ndokos
Copy link
Member

@ndokos ndokos commented Apr 12, 2023

Fixes #3374

This is a continuation of #3348, implementing fixes to the pbench_results_push tests. The main one is to subsume some exception tests under the parametrized "normal" case and eliminate redundant tests.

Fixes distributed-system-analysis#3374

This is a continuation of distributed-system-analysis#3348, implementing fixes to the
pbench_results_push tests. The main one is to subsume some exception
tests under the parametrized "normal" case and eliminate redundant
tests.
@ndokos ndokos self-assigned this Apr 12, 2023
dbutenhof
dbutenhof previously approved these changes Apr 12, 2023
Copy link
Member

@dbutenhof dbutenhof left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice cleanup; just a few consistency suggestions...

lib/pbench/test/unit/agent/task/test_results_push.py Outdated Show resolved Hide resolved
lib/pbench/test/unit/agent/task/test_results_push.py Outdated Show resolved Hide resolved
webbnh
webbnh previously approved these changes Apr 12, 2023
Copy link
Member

@webbnh webbnh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I concur with Dave's assessment -- nice clean-up -- and his comments; and here are a few more nits, if you're so inclined.

lib/pbench/test/unit/agent/task/test_results_push.py Outdated Show resolved Hide resolved
lib/pbench/test/unit/agent/task/test_results_push.py Outdated Show resolved Hide resolved
lib/pbench/test/unit/agent/task/test_results_push.py Outdated Show resolved Hide resolved
@ndokos ndokos dismissed stale reviews from webbnh and dbutenhof via 10489a1 April 12, 2023 17:01
Copy link
Member

@webbnh webbnh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@ndokos ndokos merged commit 9a57940 into distributed-system-analysis:main Apr 12, 2023
@ndokos ndokos deleted the 3374 branch April 12, 2023 19:41
ndokos added a commit to ndokos/pbench that referenced this pull request Apr 12, 2023
…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.
ndokos added a commit to ndokos/pbench that referenced this pull request Apr 13, 2023
…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.
ndokos added a commit that referenced this pull request Apr 13, 2023
* pbench-results-push: Backport of #3348 and #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.

* Add back inadvertently deleted test

test_multiple_meta_args() should not have been deleted.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Testing fixes for test_results_push.py
3 participants