[v2] Use AssertionError instead of unittest.TestCase.fail for S3 memory assertion integration tests #9873
Conversation
This test does not inherit from a unittest, so it does not have a fail method. This change makes the test raise a AssertionError with the existing failure message.
kdaily
force-pushed
the
s3-assert-max-memory-used-test-fix
branch
from
5f74af3 to
c105b78
Compare
kdaily
changed the title
aemous
left a comment
aemous
left a comment
There was a problem hiding this comment.
LGTM overall. one nit, open to discussion
|
|
||
| def assert_max_memory_used(self, process, max_mem_allowed, full_command): | ||
| peak_memory = max(process.memory_usage) | ||
| if peak_memory > max_mem_allowed: |
There was a problem hiding this comment.
non-blocking: would assert peak_memory <= max_mem_allowed, failure_message be more consistent/idiomatic with the rest of our codebase? This way it only fails when assertions are enabled
There was a problem hiding this comment.
I'm fine moving it to an idiomatic expression, but wouldn't raising an AssertionError and calling assert result in the same thing, and would fail identically if assertions are enabled?
There was a problem hiding this comment.
Yeah to clarify my comment was more about being idiomatic. Secondarily if you run it with assertions disabled then the behavior is different, this may be a moot point since PyTest enables assertions.
Replace the AssertionError with a idiomatic assert statement.
2 participants
Issue #, if available:
Description of changes:
Integration tests were ported from CLI v1, but some were altered to not inherit from a
unittest.TestCase, so it does not have a fail method. Tests using theassert_max_memory_usedwill fail incorrectly and not print the failure message. This change makes the test raise anAssertionErrorwith the existing failure message.By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.