Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Imgur Photo Exporter test #685

Merged
merged 7 commits into from
Apr 10, 2019
Merged

Imgur Photo Exporter test #685

merged 7 commits into from
Apr 10, 2019

Conversation

natlg
Copy link
Contributor

@natlg natlg commented Apr 2, 2019

Added tests for ImgurPhotoExporter ( #641)
Mockito doesn't allow to mock static fields or methods, so I refactored Exporter and Importer constructors to accept API URL instead of getting it from static variable. It was needed for mocking web server responses.
Also fixed Exception in the Importer (getting cache for a null value)

Copy link
Collaborator

@holachuy holachuy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the very thorough testing, this is very cool! I threw added a suggestion on condensing some of the assertions if we use Truth.

private static final String ALL_PHOTOS_URL_TEMPLATE =
BASE_URL + "/account/me/images/%s?perPage=" + RESULTS_PER_PAGE;
private static final String DEFAULT_ALBUM_ID = "defaultAlbumId";
private static String albumPhotosUrlTemplate;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we remove static and add final to these. That way they, after they are assigned in the constructor, the won't be modified.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

private static final String CREATE_ALBUM_URL = BASE_URL + "/album";
private static final String UPLOAD_PHOTO_URL = BASE_URL + "/image";

private static String createAlbumUrl;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Likewise, if we can remove static and make both of these final.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@natlg
Copy link
Contributor Author

natlg commented Apr 9, 2019

Hi Chuy, thanks for the review! I addressed comments, PTAL

Copy link
Collaborator

@holachuy holachuy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice! Thank you!

@holachuy holachuy merged commit 68e3980 into google:master Apr 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants