-
Notifications
You must be signed in to change notification settings - Fork 0
Conversation
simone-kalbermatter
commented
Mar 29, 2023
•
edited
Loading
edited
- Added class CollectionListAdapter to display multiple collections on the profile
- Added a button to add new collections on the profile
- Added dialog box to enter new collection name
- Changed ProfileFragment and ProfileViewmodel to adapt to these changes
- Refactored ProfileFragment
- Added tests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice job ! Also, the tests are very clean and cover most cases. I left a few comments to be addressed before we merge.
// Check that a new collection has been added to the outer RecyclerView if the user chooses a valid collection name | ||
@Test | ||
public void testAddValidCollection() throws UiObjectNotFoundException { | ||
ViewInteraction outerRecyclerView = onView(withId(R.id.collection_list_recycler_view)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a way to store these ViewInteractions as an attribute to the class or does it not work ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point I changed it.
|
||
// Check that no new collection has been added to the outer RecyclerView if the user enters an empty collection name | ||
@Test | ||
public void testEmptyCollectionNameNotAdded() throws UiObjectNotFoundException { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is valid for all the tests: Are you updating the collections in the database in the code that you're testing here ? If so, you could use the emulator in this class like in the DatabaseTests classes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code storing the collection in the database will be run, however these tests don't verify the upload to the database but only the ProfileFragment UI. The database functions should probably be tested outside of this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The database functions are already tested in another class but we should still use the emulator to avoid spamming the "production" database with test values.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I just start the database emulator before testing it won't upload to the actual database? Even if it is not in a database test?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes you can just copy the setup method from the database tests (just the part where you use the emulator ofc).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay thanks, done!
} | ||
/* holder.mediaRating.setText(review.getGrade());*/ | ||
// Set the media rating | ||
int grade = review.getGrade(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible that this may cause some error ? Since the grade attribute isn't mandatory in the declaration of a new review getGrade() might return 0 when the user simply didn't give a rating. Maybe we can choose not to display any rating when it's less than 1.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True, I changed it to display an empty string if the return value is 0.
return collections.size(); | ||
} | ||
|
||
public static class ViewHolder extends RecyclerView.ViewHolder { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Identical blocks of code found in 2 locations. Consider refactoring.
Code Climate has analyzed commit 5b5ecd2 and detected 2 issues on this pull request. Here's the issue category breakdown:
The test coverage on the diff in this pull request is 95.8% (80% is the threshold). This pull request will bring the total coverage in the repository to 77.7% (0.0% change). View more on Code Climate. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for adressing my comments, here you go :)
This reverts commit b6fc53d.