| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
|
@andrewsg can you link me to the relevant documentation section(s) to update? do you have specific wording in mind? |
Sorry, something went wrong.
|
Hi @ddelange, Thanks for your submission. I've retargeted the PR to the 3.0 branch. For safety the test runner has to be activated manually for external PRs, so I've started the tests running now. Three things:
Miscellaneous ~~~~~~~~~~~~~ - The BlobWriter class now attempts to terminate an ongoing resumable upload if the writer exits with an exception.
# no-op when nothing was uploaded yet
with pytest.raises(ValueError, match="SIGTERM received"):
with blob.open("wt") as writer:
> writer.write(b'first chunk') # not yet uploaded
E TypeError: write() argument must be str, not bytes
|
Sorry, something went wrong.
|
Hi @andrewsg 👋 I rebased my commit to the new target branch, and fixed points 1 2 and 3. Point 3 was my typo in the tests, causing them to exit early. With the test passing, point 4 should also be covered (literally :]). |
Sorry, something went wrong.
|
Thank you for doing all of that! Unfortunately, system tests exist alongside the unit tests but they aren't a replacement for unit tests in the coverage requirements for this package, so we still will need unit tests covering the lines mentioned for the PR to pass. |
Sorry, something went wrong.
|
Looks like there's another system test issue: shared_bucket = , blobs_to_delete = []
service_account =
def test_blobwriter_exit(
shared_bucket,
blobs_to_delete,
service_account,
):
blob = shared_bucket.blob("NeverUploaded")
# no-op when nothing was uploaded yet
with pytest.raises(ValueError, match="SIGTERM received"):
with blob.open("wb") as writer:
writer.write(b"first chunk") # not yet uploaded
raise ValueError("SIGTERM received") # no upload to cancel in __exit__
# blob should not exist
assert not blob.exists()
# unhandled exceptions should cancel the upload
with pytest.raises(ValueError, match="SIGTERM received"):
with blob.open("wb") as writer:
writer.write(b"first chunk") # not yet uploaded
> writer.write(b"big chunk" * 1024 ** 8) # uploaded
E OverflowError: cannot fit 'int' into an index-sized integer
Let me know if you need help with running the system tests locally. |
Sorry, something went wrong.
|
Thanks for the heads up. I'll write a new unit test case. Regarding the OverflowError: can you suggest a cleaner way to trigger the threshold where the first part is uploaded (multipart upload created)? Is there a certain amount of bytes stored in a constant I can import? To change the line to something like writer.write(b'0' * MIN_PART_UPLOAD_SIZE)? |
Sorry, something went wrong.
|
Hi ddlange, yes, overriding the chunk size makes sense to me. Go for it. |
Sorry, something went wrong.
|
@andrewsg ready for review again, now with unit tests |
Sorry, something went wrong.
|
Thank you. Looks like there are still test failures with the system test and with the unit test. Can you take a look? Run "nox -s unit-3.11" (or whatever version) to run the unit tests, subsequently "nox -s cover" to check coverage, and "nox -s blacken lint" to get lint to pass as well. |
Sorry, something went wrong.
|
@andrewsg thanks for the heads up.
unit tests are green on my machine, but nox -s cover still shows 442-445, 448-451 missing, which are the complete functions this PR adds. how can that be? the unit tests I wrote should cover all lines I added 🤔 |
Sorry, something went wrong.
|
Thank you for catching the unit test issue in the other tests. It would have been avoided if I'd used the "spec" features of the mocking library properly; my mistake. I appreciate you fixing it in the entire file. I don't know why coverage doesn't work on your machine - it's finicky - but it is clear in CI so all is good with the unit tests and coverage. The system test is still failing. The part of the test that was causing an error previously due to expanding too aggressively, which you fixed by using the chunk size as the size to write, is repeated further down in the test. |
Sorry, something went wrong.
Apply suggestions from code review
thanks for the catch, fixed now! |
Sorry, something went wrong.
|
@andrewsg can you run CI again? |
Sorry, something went wrong.
|
@ddelange Checks pass. Thank you for your contribution, and again thanks for the heads-up about the testing issue. |
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:
Fixes #1228