test: add tests for unprotected behavior#2209
Conversation
Co-authored-by: nilsreichardt <24459435+nilsreichardt@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
There was a problem hiding this comment.
Code Review
This pull request adds a comprehensive suite of unit tests for critical unprotected behavior in the hausaufgabenheft_logik domain package. The tests are well-structured and cover important functionality related to date calculations, scheduling logic, sorting, and domain constraints. My feedback focuses on improving the maintainability and readability of the new test code by reducing code duplication and replacing magic values with named constants, which will make the tests more robust and easier to manage in the future. All original comments have been retained as they do not contradict any provided rules.
| final homework = _TestHomework( | ||
| id: HomeworkId('hw-1'), | ||
| title: title, | ||
| subject: subject, | ||
| courseId: CourseId('course-1'), | ||
| withSubmissions: false, | ||
| todoDate: pastDate, | ||
| ); |
There was a problem hiding this comment.
The creation of _TestHomework is very repetitive across the tests in this group. To reduce boilerplate and improve maintainability, consider extracting the object creation into a helper function. This function could take the todoDate as a parameter, since that's the main property that varies between tests.
For example:
_TestHomework createHomework(DateTime todoDate) {
return _TestHomework(
id: HomeworkId('hw-1'),
title: title,
subject: subject,
courseId: CourseId('course-1'),
withSubmissions: false,
todoDate: todoDate,
);
}| final overdueHomework = _TestHomework( | ||
| id: HomeworkId('hw-overdue'), | ||
| title: Title('Overdue HW'), | ||
| subject: mathSubject, | ||
| courseId: CourseId('c-1'), | ||
| withSubmissions: false, | ||
| todoDate: DateTime(2023, 10, 14), // Yesterday | ||
| ); |
There was a problem hiding this comment.
There is significant repetition in the creation of _TestHomework instances for test data. To make the test setup more concise, readable, and maintainable, consider creating a helper function. This function could take parameters for the properties that vary between homework items, such as id, title, subject, and todoDate.
For example:
_TestHomework createHomework({
required String id,
required String title,
required Subject subject,
required DateTime todoDate,
}) {
return _TestHomework(
id: HomeworkId(id),
title: Title(title),
subject: subject,
courseId: CourseId('c-1'),
withSubmissions: false,
todoDate: todoDate,
);
}|
|
||
| expect(result.length, 2); // Math and English | ||
|
|
||
| final englishSection = result.firstWhere((s) => s.title == 'English'); |
There was a problem hiding this comment.
To avoid using a magic string and to make the test more robust against changes to the test data, it's better to use englishSubject.name here instead of the literal 'English'. This ensures that if the subject name in the test setup changes, this check will automatically update.
| final englishSection = result.firstWhere((s) => s.title == 'English'); | |
| final englishSection = result.firstWhere((s) => s.title == englishSubject.name); |
| expect(englishSection.homeworks.contains(todayHomework), isTrue); | ||
| expect(englishSection.homeworks.contains(in2DaysHomework), isTrue); | ||
|
|
||
| final mathSection = result.firstWhere((s) => s.title == 'Math'); |
There was a problem hiding this comment.
To avoid using a magic string and to make the test more robust against changes to the test data, it's better to use mathSubject.name here instead of the literal 'Math'. This ensures that if the subject name in the test setup changes, this check will automatically update.
| final mathSection = result.firstWhere((s) => s.title == 'Math'); | |
| final mathSection = result.firstWhere((s) => s.title == mathSubject.name); |
| // 14th = Sat (6), 15th = Sun (7), 16th = Mon (1), 17th = Tue (2), 18th = Wed (3) | ||
| expect(result.length, 5); | ||
|
|
||
| final mondaySection = result.firstWhere((s) => s.weekday == 1); |
There was a problem hiding this comment.
Using the magic number 1 for Monday is less readable than using the DateTime.monday constant. Using the constant makes the code more self-documenting.
| final mondaySection = result.firstWhere((s) => s.weekday == 1); | |
| final mondaySection = result.firstWhere((s) => s.weekday == DateTime.monday); |
| final mondaySection = result.firstWhere((s) => s.weekday == 1); | ||
| expect(mondaySection.homeworks.single, tomorrowHomework); // 16th | ||
|
|
||
| final tuesdaySection = result.firstWhere((s) => s.weekday == 2); |
There was a problem hiding this comment.
Using the magic number 2 for Tuesday is less readable than using the DateTime.tuesday constant. Using the constant makes the code more self-documenting.
| final tuesdaySection = result.firstWhere((s) => s.weekday == 2); | |
| final tuesdaySection = result.firstWhere((s) => s.weekday == DateTime.tuesday); |
| final tuesdaySection = result.firstWhere((s) => s.weekday == 2); | ||
| expect(tuesdaySection.homeworks.single, in2DaysHomework); // 17th | ||
|
|
||
| final wednesdaySection = result.firstWhere((s) => s.weekday == 3); |
There was a problem hiding this comment.
Using the magic number 3 for Wednesday is less readable than using the DateTime.wednesday constant. Using the constant makes the code more self-documenting.
| final wednesdaySection = result.firstWhere((s) => s.weekday == 3); | |
| final wednesdaySection = result.firstWhere((s) => s.weekday == DateTime.wednesday); |
| final wednesdaySection = result.firstWhere((s) => s.weekday == 3); | ||
| expect(wednesdaySection.homeworks.single, futureHomework); // 18th | ||
|
|
||
| final saturdaySection = result.firstWhere((s) => s.weekday == 6); |
There was a problem hiding this comment.
Using the magic number 6 for Saturday is less readable than using the DateTime.saturday constant. Using the constant makes the code more self-documenting.
| final saturdaySection = result.firstWhere((s) => s.weekday == 6); | |
| final saturdaySection = result.firstWhere((s) => s.weekday == DateTime.saturday); |
| final saturdaySection = result.firstWhere((s) => s.weekday == 6); | ||
| expect(saturdaySection.homeworks.single, overdueHomework); // 14th | ||
|
|
||
| final sundaySection = result.firstWhere((s) => s.weekday == 7); |
There was a problem hiding this comment.
Using the magic number 7 for Sunday is less readable than using the DateTime.sunday constant. Using the constant makes the code more self-documenting.
| final sundaySection = result.firstWhere((s) => s.weekday == 7); | |
| final sundaySection = result.firstWhere((s) => s.weekday == DateTime.sunday); |
|
Visit the preview URL for this PR (updated for commit d075177): https://sharezone-console-dev--pr2209-jules-homework-domai-1mmlj753.web.app (expires Wed, 11 Mar 2026 16:49:31 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: 471536afe3f6ec4895d9ea75513730b515d17eb6 |
|
Visit the preview URL for this PR (updated for commit d075177): https://sharezone-website-dev--pr2209-jules-homework-domai-wwd1k1ic.web.app (expires Wed, 11 Mar 2026 16:49:40 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: 372b0431a96247f908d9a97d5d865de1c8b3b04e |
|
Visit the preview URL for this PR (updated for commit d075177): https://sharezone-test--pr2209-jules-homework-domai-39r9ni9y.web.app (expires Wed, 11 Mar 2026 16:50:35 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: 4cb3ae61e1e018abfd9841fd3239f5b49ccc034b |
Co-authored-by: nilsreichardt <24459435+nilsreichardt@users.noreply.github.com>
Added unit tests for critical unprotected behavior in the
hausaufgabenheft_logikdomain package.What behavior is now protected:
clock, and day additions handling month/year boundaries (inDatemodel).BaseHomeworkReadModelisOverdueRelativeTocheck to accurately determine if homework is past due relative to the active date.HomeworkSortAndSubcategorizerlogic mapping sorting strategies (SmallestDateSubjectAndTitleSort,SubjectSmallestDateAndTitleSort,WeekdayDateSubjectAndTitleSort) to distinct list sections (e.g. 'Heute', 'Überfällig').Subjectwith an empty name properly throws anArgumentError.What bugs it would catch:
PR created automatically by Jules for task 3608123647689827036 started by @nilsreichardt