Cache Key now takes Git's Raw File URL Mask into account
Review Request #7928 — Created Jan. 30, 2016 and submitted
After initializing a Git repository and adding a bad Raw File URL Mask, the retrieved file is cached. After the Raw File URL Mask is changed to a valid one, the retrieved file is still from the cache because it does not take into consideration the new Raw File URL.
This was fixed by including the raw_file_url into the cache key for the functions:
_make_file_exists_cache_keyand_make_file_exists_cache_key. By including the raw_file_url into the cache key, the cache key will change whenever the raw file url is changed, such that it will not retrieve stale files from the cache. This change was included inreviewboard/scmtools/models.py.
Added unit tests to check that the cached copy of the file is grabbed when there are no changes to the raw_file_url and to check that a new file is retrieved when the raw_file_url changes.
Added unit test for get_file_exists() function.Ran whole test suite- all tests pass.
| Description | From | Last Updated |
|---|---|---|
|
Single quotes for strings. |
 brennie
|
|
|
The two strings should line up, e.g.: ... = ('http://github.com...' 'reviewboard/....') |
brennie
|
|
|
Single quotes for strings. |
brennie
|
|
|
Col: 38 E128 continuation line under-indented for visual indent |
 reviewbot
|
|
|
Col: 38 E128 continuation line under-indented for visual indent |
reviewbot
|
|
|
Col: 45 E128 continuation line under-indented for visual indent |
reviewbot
|
|
|
Col: 45 E128 continuation line under-indented for visual indent |
reviewbot
|
|
|
Col: 80 E501 line too long (83 > 79 characters) |
reviewbot
|
|
|
Col: 80 E501 line too long (81 > 79 characters) |
reviewbot
|
|
|
Col: 80 E501 line too long (81 > 79 characters) |
reviewbot
|
|
|
Col: 80 E501 line too long (89 > 79 characters) |
reviewbot
|
|
|
Col: 80 E501 line too long (81 > 79 characters) |
reviewbot
|
|
|
Col: 63 W291 trailing whitespace |
reviewbot
|
|
|
Col: 28 W291 trailing whitespace |
reviewbot
|
|
|
Col: 28 W291 trailing whitespace |
reviewbot
|
|
|
Col: 28 W291 trailing whitespace |
reviewbot
|
|
|
This can be None, so we need to normalize like above. (Same with the code below.) We're getting to the … |
 chipx86
|
|
|
How about: """Testing Repository.get_file re-fetches when raw file URL changes""" |
chipx86
|
|
|
Here's a way to enhance the test and make it more thorough, self-explanatory, and future-proof: Return a fake result in … |
chipx86
|
|
|
Col: 31 E251 unexpected spaces around keyword / parameter equals |
reviewbot
|
|
|
Col: 31 E251 unexpected spaces around keyword / parameter equals |
reviewbot
|
|
|
Col: 9 E265 block comment should start with '# ' |
reviewbot
|
|
|
Col: 9 E265 block comment should start with '# ' |
reviewbot
|
|
|
I'd like to see this have a few more comments, to explain exactly what the order of operations are. Comments … |
 david
|
KS
- Bugs:
-
Tool: PEP8 Style Checker Processed Files: reviewboard/scmtools/tests.py reviewboard/scmtools/models.py -
-
-
-
-
-
-
-
-
-
Tool: Pyflakes Processed Files: reviewboard/scmtools/tests.py reviewboard/scmtools/models.py Tool: PEP8 Style Checker Processed Files: reviewboard/scmtools/tests.py reviewboard/scmtools/models.py -
-
-
-
-
Tool: Pyflakes Processed Files: reviewboard/scmtools/tests.py reviewboard/scmtools/models.py Tool: PEP8 Style Checker Processed Files: reviewboard/scmtools/tests.py reviewboard/scmtools/models.py
-
Make sure to follow this guide on writing good summary/description content: https://www.reviewboard.org/docs/codebase/dev/writing-good-descriptions/
I also want to see a unit test for
get_file_exists(). -
This can be
None, so we need to normalize like above. (Same with the code below.)We're getting to the point now, though, where this is going to hit the max line length. So how about:
return 'file:...............' % ( self.pk, urlquote(...), urlquote(...), ... )(One per line.)
That'll help make this more readable. (Also, if you switch to single line quotes for this string while you're making this change, it'll help get our codebase more consistent.)
-
-
Here's a way to enhance the test and make it more thorough, self-explanatory, and future-proof:
- Return a fake result in your
spy_on. - Call and check the output.
- Change the fake result.
- Call and check that the output remains what it was before. (This ensures that we are in fact grabbing the cached copy, protecting us from test infrastructure failure or other weirdness.)
- Change the raw file URL.
- Call and check that we get the value from step 3.
- Return a fake result in your
KS
- Testing Done:
-
+ What was wrong
+ After initializing a Git repository and adding a bad Raw File URL Mask, the retrieved file is cached. After the Raw File URL Mask is changed to a valid one, the retrieved file is still from the cache because it does not take into consideration the new Raw File URL.
~ This was fixed by including the raw_file_url into the cache key for the functions:
_make_file_exists_cache_keyand_make_file_exists_cache_key. This change was included inreviewboard/scmtools/models.py. An automated test is included to check for two separate calls to the_get_file_uncachedfunction, since the function should be invoked whenever theraw_file_urlis changed.~ How it was fixed
+ + This was fixed by including the raw_file_url into the cache key for the functions:
_make_file_exists_cache_keyand_make_file_exists_cache_key. By including the raw_file_url into the cache key, the cache key will change whenever the raw file url is changed, such that it will not retrieve stale files from the cache. This change was included inreviewboard/scmtools/models.py.+ + Testing
+ + Added unit tests to check that the cached copy of the file is grabbed when there are no changes to the raw_file_url and to check that a new file is retrieved when the raw_file_url changes. All tests pass.
-
Tool: Pyflakes Processed Files: reviewboard/scmtools/tests.py reviewboard/scmtools/models.py Tool: PEP8 Style Checker Processed Files: reviewboard/scmtools/tests.py reviewboard/scmtools/models.py -
-
-
-
-
Tool: PEP8 Style Checker Processed Files: reviewboard/scmtools/tests.py reviewboard/scmtools/models.py
-
Hi Kevin,
The info on what was wrong and how it was fixed should go in the description of the change. It shouldn't have its own headers, but just be a complete story. Take a look at the examples in that guide and you'll see what we expect. You can also dig into other commits by David and myself to see some other examples.
KS
- Description:
-
~ Incorporated raw_file_url into cache key in Repository class
~ After initializing a Git repository and adding a bad Raw File URL Mask, the retrieved file is cached. After the Raw File URL Mask is changed to a valid one, the retrieved file is still from the cache because it does not take into consideration the new Raw File URL.
+ + This was fixed by including the raw_file_url into the cache key for the functions:
_make_file_exists_cache_keyand_make_file_exists_cache_key. By including the raw_file_url into the cache key, the cache key will change whenever the raw file url is changed, such that it will not retrieve stale files from the cache. This change was included inreviewboard/scmtools/models.py. - Testing Done:
-
~ What was wrong
~ Added unit tests to check that the cached copy of the file is grabbed when there are no changes to the raw_file_url and to check that a new file is retrieved when the raw_file_url changes.
+ Added unit test for get_file_exists() function. ~ After initializing a Git repository and adding a bad Raw File URL Mask, the retrieved file is cached. After the Raw File URL Mask is changed to a valid one, the retrieved file is still from the cache because it does not take into consideration the new Raw File URL.
~ Ran whole test suite- all tests pass.
- - How it was fixed
- - This was fixed by including the raw_file_url into the cache key for the functions:
_make_file_exists_cache_keyand_make_file_exists_cache_key. By including the raw_file_url into the cache key, the cache key will change whenever the raw file url is changed, such that it will not retrieve stale files from the cache. This change was included inreviewboard/scmtools/models.py.- - Testing
- - Added unit tests to check that the cached copy of the file is grabbed when there are no changes to the raw_file_url and to check that a new file is retrieved when the raw_file_url changes. All tests pass.
Tool: Pyflakes Processed Files: reviewboard/scmtools/tests.py reviewboard/scmtools/models.py