Conversation
…ing is working as intended
…ce testing since that is tested in the integration test file
| import org.junit.Assert | ||
| import org.junit.Test | ||
|
|
||
| class PostUnitTest { |
There was a problem hiding this comment.
Having Post DTO use its own file prevents testing from getting cluttered into a single file
| @get:Rule | ||
| var rule: TestRule = InstantTaskExecutorRule() | ||
|
|
||
| lateinit var mvm : MainViewModel | ||
|
|
||
| private val mainThreadSurrogate = newSingleThreadContext("Main thread") | ||
|
|
||
| @MockK | ||
| lateinit var mockUniService : UniversityService | ||
|
|
||
|
|
||
| lateinit var universityService: IUniversityService | ||
| var allUniversities : List<University>? = ArrayList<University>() |
There was a problem hiding this comment.
none of these things were actually being used, leaving a lot of code sitting around, not contributing
There was a problem hiding this comment.
Are you certain they are not being used?
It looks like a valid unit test to me.
| private fun thenTheUniversityCollectionShouldContainMarywoodUniversity() { | ||
| Assert.assertNotNull(allUniversities) | ||
| allUniversities?.let { | ||
| Assert.assertTrue(it.isNotEmpty()) | ||
| var containsMarywoodUniversity = false |
There was a problem hiding this comment.
Test to ensure not only that the collection is not empty but that the contents of items in the collection are not empty
Use let to avoid a forced unwrap situation
| } | ||
| } | ||
| Assert.assertTrue(containsMarywoodUniversity) | ||
| } ?: Log.e(TAG, "allUniversities was null") |
There was a problem hiding this comment.
Using Log.e here to print to the logs in the instance that allUniversities is somehow null. This shouldn't happen because we already did a test for that, but as part of implementing the let statement, I believe something is needed in the case that it somehow is null, so I included this
| /*allPosts.add(Post(NEW_SPECIMEN))*/ | ||
| /*allPosts.add(Post(NEW_SPECIMEN))*/ |
There was a problem hiding this comment.
As long as you're here, remove this entirely.
Avoid pushing commented code to Version Control. Version control has tools that makes this unnecessary.
- You can look at history to see what used to be there.
- You can use branches for experimental features.
|
From this review:
|
I should have not committed the formatting part, it was done by the IDE and I simply forgot about those changes and committed it. The main things I did were separating tests into 3 files: Previously tests for all 3 of these things were housed in a single file which was a little bit hard to read I also replaced forced unwraps in the integration testing file with let statements and added an additional test |
2 participants
Analysis of the App
This app is a social media app where college students can join a community of users at the university, get news for the school, and join groups for things like sports teams or clubs. The app builds and runs fine. So far the UI appears to have fields for adding posts and the ability to log in, so multiple core functionalities have already been implemented.
Code Availability and Buildability
The code was available on GitHub on time, builds and runs.
Documentation
The documentation is enough that I have a general idea what is going on and what they plan to do in the future. I can also figure out specifics fairly quickly by just using context.
3 Things I learned
Changes I made
Rationale for Changes
These changes make the tests into their own separate files so it's easier to work with and less confusing to read. I also add some test coverage by ensuring retrofit parses actual university objects instead of an array of empty strings. It did properly parse though so looking good!
3 Commits from my own group project