Fixed s3 to s3 sync url decoding pagniation bug. #909
Conversation
Keys were not getting url decoded properly with pagination. If one bucket exited ListObjects pagination before the other bucket, it caused the handler that decodes the keys to prematurely exit as well. This resulted in some keys not getting url decoded.
|
How does my coverage decrease when the report says that the coverage increased for the file I changed? It looks like for the other builds |
|
Can't seem to find this in coveralls, is there a page that actually shows the diff in coverage? |
|
Yes if you click the black and green button that says 90% in the coveralls comment. |
|
I'm asking for a page that shows the diff in coverage. Coveralls says it dropped 0.01%. Where does it show this? Clicking on the link shows everything is increasing. You mentioned something about configure.py. Where does it show that?
|
|
Click on either job 1222.1() or 1222.2() in the jobs column. |
|
Sorry, I don't think I'm being clear. In order to answer your initial question, I need to be able to see the lines of code for which coveralls is claiming a drop in coverage, which is what I've been referring to as a diff. Maybe I'm missing something obvious, but I can only see what lines of code are covered/not covered. I can't see what lines changed from covered->not covered. Am I correct in assuming I have to just pick an earlier build and manually scroll line by line to see the diff? Perhaps it would just be more expedient if you can show what lines of code are not being covered anymore that previously were being covered, and we can go from there. |
|
Oh I don't know how to find a diff either. I can only see covered and not covered as well. Manually scrolling is the only way that I can tell. |
|
I just compared the code between my pull request and the recently merged doc update branch and they looked the exact same... |
|
The difference isn't obvious to me. It's possible this is a Coveralls bug maybe? Some lines change depending on Python version - maybe it has some issue with that? |
| @@ -490,20 +490,29 @@ def test_sync_with_plus_chars_paginate(self): | |||
| self.assertNotIn('upload:', p2.stdout) | |||
| self.assertEqual('', p2.stdout) | |||
|
|
|||
| def test_s3_to_s3_sync_with_plus_char(self): | |||
| self.files.create_file('foo+.txt', contents="foo") | |||
| def test_s3_to_s3_sync_with_plus_char_paginate(self): | |||
There was a problem hiding this comment.
Is it not worth keeping the original test? It might be useful to verify both the non-paginated and paginated cases.
There was a problem hiding this comment.
I felt that the test that I added encompasses the non-paginated case since we always use a paginate call when listing the objects in a bucket. The main benefit in keeping the test is that it adds more granularity such that we would know if it is the first paged that is not url decoded or the pages after it. The reason I took it out in the first place is that it saves time in running the test. I estimated the test takes around 20 seconds. So I figured since the logic was the same, we could save some time/reduce redundancy.
|
Small question about, but other than that, looks good. |
|
|
Looks good.Fixed s3 to s3 sync url decoding pagniation bug.
Keys were not getting url decoded properly with pagination.
If one bucket exited ListObjects pagination before
the other bucket, it caused the handler that decodes the keys
to prematurely exit as well. This resulted in some keys not
getting url decoded.
cc @jamesls @danielgtaylor