found some mistakes #3

SimonZajacc · 2023-04-17T16:01:03Z

Tieto testy mi neprechádzajú a domnievam sa že to tak nemá byť.
Commitnuty kod - pri každom teste, ktorý mi nezbehol som buď zmenil(okomentoval) riadky prislúchajúce chybám alebo opravil ak sa to dalo bez väčších zmien.

StudentTest - ShouldUpdateStudent - metóda getStudent(aisId) vráti null, čo je v mojom prípade očakávané keďže používam id ako identifikátor v entite Student
- vyriešil by som to hľadaním študenta podľa id(ilustračná zmena v kóde, viem že to nezbehne)
TeacherTest - ShouldUpdateTeacher - to iste čo v prvom prípade, vyhľadavánie Teachera pomocou aisId
- vyriešil by som to hľadaním Teachera podľa id(ilustračná zmena v kóde)
ThesisTest - ShouldUpdateThesis - pri updatovani thesis som zakázal meniť učiteľa(beriem to ako pokus o krádež prace)
ShouldAssignThesisToStudentWithAlreadyAssignedThesis() - úmyselne som tiež zakázal zapísanie si inej prace študentovi, keďže už prácu ma zapísanú... riešilo so to na dc že to nemusíme riešiť
PageableTest - shouldGetPageOfStudents() - pri poslednom volaní findStudents() je nastavené pageable na page = 1, size = 5 a hľadáme študentov podľa grade = 3, študentov je síce 5 podľa grade = 3 ale keďže pageable chce 1-vú stránku a nie 0-ltú tak hneď pri prvom asserte by sme mali očakávať 5 a nie 0
- keďže je v pageable nastavená stránka 1 na vstupe musíme zmeniť aj posledný assert z 0 na 1(zabudol som to dať do commitu spolu) => assertEquals(1, students.getPageable().getPageNumber());

Ak sa mýlim, v niečom a bolo toto zbytočné tak sa vopred ospravedlňujem

tuplle · 2023-04-17T23:20:44Z

src/test/java/sk/stuba/fei/uim/vsa/pr1/StudentTest.java

@@ -140,7 +140,7 @@ void ST04_shouldUpdateStudent() {
            student = thesisService.updateStudent(student);
            assertNotNull(student);
            assertEquals(randomSetStringForTesting, getFieldValue(student, stringField, String.class));
-            student = thesisService.getStudent(Student01.aisId);
+            student = thesisService.getStudent(Student01.id);


Student01 je trieda v súbore TestData, ktorá obsahuje iba súbor testovacích hodnôt, nemá nič s tím ako je implementovaná entita a už vôbec nemá atribút id.
Spúšťal si si to s tvojimi zmenami?

Viem ale nevedel som ako to opraviť tak som to aspoň ilustroval aby ste vedeli o chybe.

tuplle · 2023-04-17T23:21:03Z

src/test/java/sk/stuba/fei/uim/vsa/pr1/TeacherTest.java

@@ -140,7 +140,7 @@ void TE04_shouldUpdateTeacher() {
            teacher = thesisService.updateTeacher(teacher);
            assertNotNull(teacher);
            assertEquals(randomSetStringForTesting, getFieldValue(teacher, stringField, String.class));
-            teacher = thesisService.getTeacher(Teacher01.aisId);
+            teacher = thesisService.getTeacher(Teacher01.id);


Rovnaké ako v komentári vyššie pri Student01.

tuplle · 2023-04-17T23:22:49Z

src/test/java/sk/stuba/fei/uim/vsa/pr1/ThesisTest.java

@@ -508,7 +508,7 @@ void TH09_shouldUpdateThesis() {
            String teacherField = teacherFields.get(0);
            assertNotNull(getFieldValue(thesis, teacherField, teacherClass));
            assertEquals(teacher01Id, getEntityId(getFieldValue(thesis, teacherField, teacherClass), teacherIdField));
-            thesis = setFieldValue(thesis, teacherField, teacher02);
+            thesis = setFieldValue(thesis, teacherField, teacher02);  // zakazal som kradnutie prace a teda zmenu ucitela


Nebral by som to ako kradnutie práce. Zmena vedúceho práce je bežná záležitosť, ktorá má svoje opodstatnenie. Nakoľko učiteľ je iba jeden z atribútov práce, ak zachováme, že môže byť v práci uvedený práve jeden je to absolútne v poriadku.

Ja som to tak žiaľ bral, tak teda som to mal asi tušiť ospravedlňujem sa za zbytočný pull request.

tuplle · 2023-04-17T23:23:47Z

src/test/java/sk/stuba/fei/uim/vsa/pr1/ThesisTest.java

@@ -206,7 +206,7 @@ void TH02_1_shouldAssignThesisToStudentWithAlreadyAssignedThesis() {
            testToHaveAnIdField(thesis02, thesisIdField);
            Long thesis02Id = getEntityId(thesis02, thesisIdField);

-            thesis02 = thesisService.assignThesis(thesis02Id, studentId);
+            thesis02 = thesisService.assignThesis(thesis02Id, studentId); // toto som zakazal, prislo mi to logicke


Je to možné že sa to riešilo musím však nájsť kde presne.

tuplle · 2023-04-17T23:27:19Z

src/test/java/sk/stuba/fei/uim/vsa/pr1/bonus/PageableTest.java

@@ -113,7 +113,7 @@ void shouldGetPageOfStudents() throws NoSuchFieldException, IllegalAccessExcepti
        students = pagedThesisService.findStudents(Optional.empty(), Optional.of("3"), pageable);
        assertNotNull(students);
        assertNotNull(students.getContent());
-        assertEquals(5, students.getContent().size());
+        assertEquals(0, students.getContent().size());


Toto nie je dobre. Z popisu PR je zrejmé, že metódu Pageable.next() máš zrejme zle implementovanú nakoľko tá metóda má vrátiť novú inštanciu Pageable nie upraviť pôvodnú. Takže posledné volanie findStudents() pracuje práve s Pageable(0, 5).

Ah tak, tak to som zle vydedukoval, do budúcna by som možno urobil podrobnejší popis zadania. Viem, že to asi nie je jednoduché myslieť na všetko ale bolo tam veľa veci čo sme museli sami dedukovať/pýtať sa na dc a to je čas a práca navyše zase pre vás.

found some mistakes

c6b1b25

tuplle requested changes Apr 17, 2023

View reviewed changes

tuplle assigned SimonZajacc Apr 17, 2023

tuplle added the bug Something isn't working label Apr 17, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

found some mistakes #3

found some mistakes #3

SimonZajacc commented Apr 17, 2023

tuplle Apr 17, 2023

SimonZajacc Apr 18, 2023

tuplle Apr 17, 2023

tuplle Apr 17, 2023

SimonZajacc Apr 18, 2023

tuplle Apr 17, 2023

tuplle Apr 17, 2023

SimonZajacc Apr 18, 2023

found some mistakes #3

Are you sure you want to change the base?

found some mistakes #3

Conversation

SimonZajacc commented Apr 17, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment