-
Notifications
You must be signed in to change notification settings - Fork 3
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
implement and write tests for GeneralSkillsRepo #24
Conversation
dont merge, for some reason I can not create Draft Pull Request |
package com.earth.testomania.tests.general | ||
|
||
class GeneralSkillsMathematicalRepo { | ||
|
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.
I think it's better to have some interface for repo and it's implementation
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.
I know most of the books/blogspots say that, including you, but I disagree... So many interfaces and so many implementations I have seen and almost none used the benefits of having an interface. 99% of those did not had more than one implementation.
Actually, I want to deliberately make a mistake and do it without interfaces. It will be interesting to see how bad is this tight coupling thing.
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.
Why do you want to create mistakes on our project? :D can you do it in another project? :D
well I agree with @kartlos99 we want to use best practices and implementation of repo is best practice, I know sometimes it seems we are adding a level of complexity and it may never change and be needed, but and I mean BIG BUT what if change will be nececery and interface will not be there? we will be disappointed :( so I also recomd to use best practices and thus use interface and implementation of the interface
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.
| sometimes it seems we are adding a level of complexity
ჩემთვის ძალიან ხშირად ასე ჩანს.... რეალურად გვინდა რო მივყვეთ მაგ ბესთ ფრაქთისს? მე ვერ ვხედავ საჭიროებას.
| we will be disappointed
ესეც საკითხავია, მაინც რამდენად იმედგაცრუებული ვიქნებით? ინტელიჯეის უკვე აქვს ძალიან კარგი რეფაქტორის ფუნქციები, თუ მართლა გახდა საჭირო 2 წუთში შეილება დაემატოს ინტერფეისი...
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.
რადგან ჩვენი პროექტის ერთერთი მთავარი მიზანია განვითარება და სიახლეების სწავლა, შესაბამისად მისაღებია შენი გადაწყვეტილება, ანუ ნეიტალურს დავიჭერ მე, თუმცა ალბათ სხვების აზრიც უნდა მოვისმინოთ:
@nmgalo @mlkway თქვენ რას ფიქრობთ, დავტოვოთ ასე ინტერფეისის გარეშე?
მაგრამ ჩვენ ხომ შევთახმდით რომ სხვადასხვა ტესტი, თავთავის პაკეტში იქნებოდა, და როგორც მახსოვს უნარების ტესტებს სახელი skills შურჩიეთ, თუ გინდა skills_test დავარქვათ პაკეტს, მხოლოდ 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.
ჩემიაზრით ჯობია სწორ პრაქტიკას მივყვეთ, Clean code ში ეწერა ეგ LeBlanc’s law: Later equals never.
ასერომ არჯობია თავიდანვე გვქონდეს სწორი მიდგომა?
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.
Clean code ში ეწერა ეგ LeBlanc’s law: Later equals never
2019 წელს დავაყენე კამერა და ავაწყვე სერვერი windows ზე ვიდეოჩანაწერებეისვის, რათქმაუნდა იმავე წელს გადავწყვიტე რომ linux ზე გადავიყვანდი სერვერს და გაგიკვირდებათ 2022 წელია მაგრამ სერვერი ისევ windows ზეა :D რადგან მუშაობს, მეზარება linux ზე გადაყვანა :D
აგერ ჩემი შპიონი google photo დამემოწმება 2019 შია გადაღებული ფოტო :D
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.
@Nodrex NixOS დააყენე, ძაან მაგარი რაღაცაა
yes unfortunately private free repo does not give much of control over branches and pull requests |
testomania/build.gradle
Outdated
@@ -3,6 +3,7 @@ plugins { | |||
id 'kotlin-android' | |||
id 'kotlin-kapt' | |||
id 'dagger.hilt.android.plugin' | |||
id 'org.jetbrains.kotlin.plugin.serialization' version '1.6.10' |
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.
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.
აუ დამავიწყდა ;დ რეფლექსურად ეგრევე კოტლინის ლიბი დავამატე
testomania/build.gradle
Outdated
testImplementation 'org.robolectric:robolectric:4.6' | ||
testImplementation group: 'com.google.dagger', name: 'hilt-android-testing', version: '2.41' | ||
kaptTest 'com.google.dagger:hilt-android-compiler:2.41' | ||
implementation "org.jetbrains.kotlinx:kotlinx-serialization-json:1.3.2" |
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.
Please look at the task: #9
import dagger.hilt.InstallIn | ||
import dagger.hilt.components.SingletonComponent | ||
import kotlinx.serialization.json.Json |
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.
Please look at the task: #9
private val tests by lazy { | ||
val rawJson = appContext.resources.openRawResource(R.raw.general_ability_test_data) | ||
json.decodeFromStream<List<GeneralTestItemDTO>>(rawJson) | ||
} |
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.
shouldn't it be suspend function or read under coroutine?
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.
მემგონი ეგ ჯობია იუზქეისებიდან ვქნათ და მანდ დავტოვოთ როგორც არის. არა?
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.
use case ში 1 suspend ფუნქცია უნდა იყოს. კორუტინი ისევ viewmodel ში უნდა იყოს
@@ -0,0 +1,19 @@ | |||
package com.earth.testomania.tests.general.dto | |||
|
|||
import kotlinx.serialization.Serializable |
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.
Please look at the task: #9
@@ -0,0 +1,12 @@ | |||
package com.earth.testomania.tests.general.dto | |||
|
|||
import kotlinx.serialization.Serializable |
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.
Please look at the task: #9
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.
არ გვინდა ამდენი ერთიდაიგივე კომენტარები :D
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.
არ გვინდა ამდენი ერთიდაიგივე კომენტარები :D
ყველგან რო ჩავანცვლოთ და არსად გამოგვრჩეს :D
|
||
@Test | ||
fun `can read general_ability_test_data json file`() { | ||
assert(repo.getAllTests().size == 40) |
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.
I would suggest using "com.google.truth:truth:1.1.3" lib, I thnk it is more readable:
- assertThat(it).isEqualTo(result)
- assertThat(it).isIn(1..Int.MAX_VALUE)
- assertThat(data).containsExactly(val1, val2, ... val3)
- assertThat(data).hasSize(1)
@shalva97 @kartlos99 @nmgalo @mlkway please share your opinions
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.
I agree with @Nodrex it's much better.
@shalva97 საქაღალდეების სტრუქტურა დავარეფაქტროინგოთ რა და დავმერჯოთ დღეს |
No description provided.