v3/animations API#7911
Conversation
Tests clean up better including delete markers Use/extract helpers for versions test Remove unneeded parentheses Test replacing an animation version Give animations its own POST endpoint
Check app quotas on copy and handle not found case
| end | ||
|
|
||
| def test_nonexistent_animation | ||
| filename = randomize_filename('nonexistent.png') |
There was a problem hiding this comment.
Style/IndentationWidth: Use 2 (not 4) spaces for indentation.
|
Hi Brad, Sorry I didn't get to this today. It is first on my list for the morning, Dave On Mon, Apr 18, 2016 at 10:29 AM, Brad Buchanan notifications@github.com
|
|
Looking... |
| when 'animations' | ||
| # Only allow specific image types to be uploaded by users. | ||
| # Only png for now, will expand support in the future | ||
| %w(.png).include? extension.downcase |
There was a problem hiding this comment.
This switch statement "smells" like it belongs in the bucket implementation. Extra credit if you agree and want to move it there. Supporting evidence: http://c2.com/cgi/wiki?SwitchStatementsSmell "Typically, similar switch statements are scattered throughout a program"
There was a problem hiding this comment.
Good call. The only other case endpoint I see in this file is the caching times (line 127), maybe I'll give that the same treatment.
| # Create or replace an animation. We use this method so that IE9 can still | ||
| # upload by posting to an iframe. | ||
| # | ||
| post %r{/v3/(animations)/([^/]+)/([^/]+)$} do |endpoint, encrypted_channel_id, filename| |
There was a problem hiding this comment.
It's not clear that POSTing rather than PUTting (which would be more RESTful) is truly necessary here, since the uploads should be same-origin. Probably a good time to address this is after we stop supporting IE9. No action required now as grepping for IE9 will find this comment.
There was a problem hiding this comment.
I also figured POST was the default method used by the jQuery upload extension we are using for asset uploads, for backwards-compatibility reasons. I am reusing that plugin to get uploads up and running quickly for now. I figure this has to be revisited at some point, as we'll probably be doing more client-side preprocessing of uploads in the future.
Before: POST v3/animations/<dest>/from/<src> After: PUT v3/animations/<dest>?src=<src>
| files_api = Rack::Test::Session.new(files_mock_session) | ||
|
|
||
| [channels_api, files_api] | ||
| end |
There was a problem hiding this comment.
A better init strategy is to use build_rack_mock_session as shown in this PR: #7410 . Since you are basically copying AssetsTest and SourcesTest, I think it's ok to continue following the the init_apis pattern here. However, can you please add a comment to all 3 tests discouraging copying of init_apis and encouraging use of build_rack_mock_session?
There was a problem hiding this comment.
Eh... if I'm going to add a comment, I might as well go clean up the code, right?
| copy_source = @bucket + '/' + s3_path(owner_id, channel_id, source_filename) | ||
| @s3.copy_object(bucket: @bucket, key: key, copy_source: copy_source) | ||
|
|
||
| # TODO (bbuchanan) |
There was a problem hiding this comment.
Style/CommentAnnotation: Annotation keywords like TODO should be all upper case, followed by a colon, and a space, then a note describing the problem.
There was a problem hiding this comment.
😕 Seriously, we lint the style of our TODO comments?
| def randomize_filename(filename) | ||
| basename = [filename.split('.')[0], '.' + filename.split('.')[1]] | ||
| basename[0] + '_' + @random.bytes(10).unpack('H*')[0] + basename[1] | ||
| end |
There was a problem hiding this comment.
It seems like whole bunch of duplicated code could be eliminated from these helper functions by having these 3 tests inherit from a common FilesApiTest(?) subclass. Would you consider getting one started, even if only some of the helpers can be easily moved there at this time?
|
Great API and tests, Brad! I know the Assets and Sources tests were a bit of a mess when you found them, so please consider any cleanup which touches them to be very optional. |
| end | ||
|
|
||
| def post_file_version(endpoint, channel_id, filename, version_id, file_contents, content_type) | ||
| body = { files: [ create_uploaded_file(filename, file_contents, content_type) ] } |
There was a problem hiding this comment.
Style/SpaceInsideBrackets: Space inside square brackets detected.
| $:.unshift File.expand_path('../lib', __FILE__) | ||
| $:.unshift File.expand_path('../shared/middleware', __FILE__) | ||
| $LOAD_PATH.unshift File.expand_path('../lib', __FILE__) | ||
| $LOAD_PATH.unshift File.expand_path('../shared/middleware', __FILE__) |
|
@davidsbailey PTAL. I refactored a bunch of setup and logic out of the sources, assets and animations tests into a common base test class. I've done quite a bit of 'boilerplate' currying of helpers for each endpoint which is a bit verbose, but I think it keeps the tests themselves easier to read and puts some good pressure on us to keep the APIs similar, which I suspect is good for our API design overall. |
|
Hi Brad, per offline discussion it sounds like you might be continuing to experiment with extracting a helper. Whether you decide to proceed with that or not, please let me know when you're ready for another look. |
|
Okay Dave, PTAL! I think this is cleaner overall. |
|
|
||
| ensure_aws_credentials(channel_id) | ||
| channel_id = create_channel | ||
| api = FilesApiTestHelper.new(current_session, 'assets', channel_id) |
There was a problem hiding this comment.
These two lines (or a slight variation) appear at the beginning of many different tests. It seems like you could extract definition of @channel_id and @api to setup, even if these don't get used in every test
|
Great extractions Brad! 💯 LGTM! It is a little hard to figure out at first why the test base and the test helper need to be separate, but I do understand the need -- only the helper has the channel id, and some tests need multiple channel ids. I feel like you've explained pretty well what the test helper is, but it might be worth a short comment telling the author of a new helper function whether to put it into test base or test helper. |
Add a
v3/animationsAPI that is similar to the assets and sources APIs.v3/animations/<new-object>/from/<existing-object>I gave it a suite of tests that are more or less the union of the assets and sources tests. I'm sure there are more possibly useful things to check. Please suggest more tests.