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

implement and write tests for GeneralSkillsRepo #24

Merged
merged 7 commits into from
Apr 3, 2022

Conversation

shalva97
Copy link
Collaborator

No description provided.

@shalva97
Copy link
Collaborator Author

dont merge, for some reason I can not create Draft Pull Request

package com.earth.testomania.tests.general

class GeneralSkillsMathematicalRepo {

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

Copy link
Collaborator Author

@shalva97 shalva97 Mar 30, 2022

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.

Copy link
Owner

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

Copy link
Collaborator Author

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 წუთში შეილება დაემატოს ინტერფეისი...

Copy link
Owner

@Nodrex Nodrex Mar 31, 2022

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 არა ინფორმატიული და ძაან ჯენერიკია
InkedScreenshot 2022-03-31 112835_LI

Copy link
Collaborator

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.
ასერომ არჯობია თავიდანვე გვქონდეს სწორი მიდგომა?

Copy link
Owner

@Nodrex Nodrex Mar 31, 2022

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
Inked277294242_394637039144992_485374272907831425_n_LI

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@Nodrex NixOS დააყენე, ძაან მაგარი რაღაცაა

@shalva97 shalva97 changed the title added DTO generalSkillsRepo is ready Mar 30, 2022
@shalva97 shalva97 changed the title generalSkillsRepo is ready implement and test generalSkillsRepo Mar 30, 2022
@shalva97 shalva97 changed the title implement and test generalSkillsRepo implement and write tests for GeneralSkillsRepo Mar 30, 2022
@Nodrex
Copy link
Owner

Nodrex commented Mar 30, 2022

dont merge, for some reason I can not create Draft Pull Request

yes unfortunately private free repo does not give much of control over branches and pull requests

@@ -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'
Copy link
Owner

Choose a reason for hiding this comment

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

As I remember @nmgalo researched which was the best serialization lib as of performance and the answer was Moshi.
Please look at the task: #9
you can reopen this task and argue with @nmgalo if we need another lib for serialization

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

აუ დამავიწყდა ;დ რეფლექსურად ეგრევე კოტლინის ლიბი დავამატე

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"
Copy link
Owner

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
Copy link
Owner

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

Comment on lines 18 to 21
private val tests by lazy {
val rawJson = appContext.resources.openRawResource(R.raw.general_ability_test_data)
json.decodeFromStream<List<GeneralTestItemDTO>>(rawJson)
}
Copy link
Owner

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

მემგონი ეგ ჯობია იუზქეისებიდან ვქნათ და მანდ დავტოვოთ როგორც არის. არა?

Copy link
Owner

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
Copy link
Owner

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
Copy link
Owner

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

არ გვინდა ამდენი ერთიდაიგივე კომენტარები :D

Copy link
Owner

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)
Copy link
Owner

@Nodrex Nodrex Mar 31, 2022

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

Copy link
Collaborator

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.

@Nodrex
Copy link
Owner

Nodrex commented Apr 3, 2022

@shalva97 საქაღალდეების სტრუქტურა დავარეფაქტროინგოთ რა და დავმერჯოთ დღეს
image

@shalva97 shalva97 merged commit 22ad1e0 into main Apr 3, 2022
@shalva97 shalva97 deleted the general_skillz_test branch April 3, 2022 10:10
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

4 participants