Bug fixes #237
Bug fixes #237
Conversation
|
Code Climate has analyzed commit f870c0e and detected 1 issue on this pull request. Here's the issue category breakdown:
The test coverage on the diff in this pull request is 64.0% (80% is the threshold). This pull request will bring the total coverage in the repository to 83.8%. View more on Code Climate. |
![codeclimate[bot]](https://avatars.githubusercontent.com/in/9403?s=60&v=4){kind=small}
| if(checkExistingSong()) { | ||
| alert(getString(R.string.download_already_done), getString(R.string.download_try_different), songView) | ||
| if(downloadStarted) { | ||
| Toast.makeText(applicationContext, getString(R.string.download_already_downloading), Toast.LENGTH_LONG).show() |
There was a problem hiding this comment.
Similar blocks of code found in 3 locations. Consider refactoring.
| companion object { | ||
| var downloadComplete = false | ||
| var downloadStarted = false | ||
| } |
There was a problem hiding this comment.
Why not creating a complete static module: DownloadStatus
that has
- Private Constructor()
- Then Companion Object(){
setDownloadComplete()
setDownloadStarted()
isDownloadComplete()
isDownloadStarted() - }
It is quite quick to write and easily testable like that it will improve the coverage :D
There was a problem hiding this comment.
Testing the static fields is done implicitly. They are there to simply be used in the tests, not to be tested by theirselves :/ It would otherwise be "I am downloading thus I am downloading" rather than a check if the download has started, which is implicitly tested in the currently commented tests. Those tests unfortunately take a long amount of time, thus they are currently commented. However, you can see the test "downloadMultipleSongs" checks for downloadComplete field, and implicitly checks the downloadStarted field.
There was a problem hiding this comment.
An important factor to mention would be that the PR does not add or remove any coverage. Although might be because of the small number of lines effected, the tests for those lines are already complete, just commented due to the reason mentioned above. Since the coverage does not decrease, I doubt it would increase either
There was a problem hiding this comment.
maybe not for the coverage, but DownloadStatus can be done for the sake of separation in general.
| @@ -21,6 +21,7 @@ import kotlinx.coroutines.launch | |||
|
|
|||
| /** | |||
| * Common set up for all games (difficulty level, progress bar) | |||
| * MISSING DOCS FOR CERTAIN METHODS INSIDE THE CLASS! | |||
There was a problem hiding this comment.
I can, but it's not a class I've written. There might be mistakes. I'll for now add them but the author of the class should go over them later on.
| } | ||
|
|
||
| val records = File(activity.applicationContext.getExternalFilesDir(Environment.DIRECTORY_DOWNLOADS), "records.txt") | ||
| assert(records.exists()) |
There was a problem hiding this comment.
why assert is used here in general?
There was a problem hiding this comment.
Because we want to make sure the file is created, i.e a download started. Also makes sure the creation of the file was successful.
| companion object { | ||
| var downloadComplete = false | ||
| var downloadStarted = false | ||
| } |
There was a problem hiding this comment.
maybe not for the coverage, but DownloadStatus can be done for the sake of separation in general.
| /* | ||
| //Test that takes too long to execute. Uncomment towards the last sprint. | ||
| @Test | ||
| fun downloadMultipleSongs() { |
There was a problem hiding this comment.
About DownloadActivity testing: I found it excessive to test if the actual song download is correct. I believe other stuff delete/permission/errors are fine to test. Maybe we should separate code that way, so actual download is note tested, but other things are.
There was a problem hiding this comment.
Currently the actual download test is commented. It is the test called "downloadCorrectSong". Any others are basically testing cases where we are either:
-trying to download a song with a non-existent name
-trying to download multiple songs at the same time (commented due to time consumption)
-trying to download a song that already exists (commented due to time consumption)
As for deletion and errors, they are tested in a different activity called DeleteSongsActivity where I am creating a text file instead of downloading an actual song and checking if deletion is working properly or not.
I do agree with you however that downloading the actual song may be overdoing it. I may remove that test since it is implicitly done in some other commented tests.
It's just an initial value. It's possible to remove it and state songName as lateinit. The initial value will be overwritten. I will be changing that when I'm adding the docs for GameManager to resolve the confusion. |
Done. The mentioned variable is now lateinit and I've completed GameManager's comments. |
Fixed the bug regarding the timer going red then orange when there are only 2 seconds left in the typing game.
Fixed the bug regarding the download activity initiating multiple downloads, currently limited to 1 download at any unit of time.