Replace randomness in tests with deterministic randomness. #1320
Conversation
WalkthroughThe overarching change across these updates focuses on standardizing the approach to generating randomness in tests across various files. This is achieved by replacing the use of different random number generation methods and the Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
github-actions
bot
added
code quality
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 2
Configuration used: CodeRabbit UI
Files ignored due to path filters (10)
app/pubspec.lockis excluded by:!**/*.lockapp/pubspec.yamlis excluded by:!**/*.yamllib/abgabe/abgabe_client_lib/pubspec.lockis excluded by:!**/*.locklib/abgabe/abgabe_client_lib/pubspec.yamlis excluded by:!**/*.yamllib/firebase_hausaufgabenheft_logik/pubspec.lockis excluded by:!**/*.locklib/hausaufgabenheft_logik/pubspec.lockis excluded by:!**/*.locklib/hausaufgabenheft_logik/pubspec.yamlis excluded by:!**/*.yamllib/test_randomness/analysis_options.yamlis excluded by:!**/*.yamllib/test_randomness/pubspec.lockis excluded by:!**/*.locklib/test_randomness/pubspec.yamlis excluded by:!**/*.yaml
Files selected for processing (19)
- app/test/dashboard/update_reminder_bloc_test.dart (2 hunks)
- app/test/feedback/feedback_bloc_test.dart (1 hunks)
- app/test/feedback/feedback_page_test.dart (1 hunks)
- app/test/groups/group_join_select_courses_bloc_test.dart (1 hunks)
- app/test/holidays/holiday_bloc_unit_test.dart (1 hunks)
- app/test/homework/homework_dialog_test.dart (1 hunks)
- app/test/homework/teacher/teacher_homework_page_widget_test.dart (1 hunks)
- app/test/timetable/school_class_filter_view_test.dart (1 hunks)
- app/test/timetable/timetable_bloc_test.dart (3 hunks)
- app/test_goldens/blackboard/details/blackboard_item_read_by_users_list_page_test.dart (3 hunks)
- app/test_goldens/calendrical_events/page/past_calendrical_events_page_test.dart (3 hunks)
- lib/abgabe/abgabe_client_lib/test/bloc_test.dart (1 hunks)
- lib/hausaufgabenheft_logik/lib/src/lehrer/teacher_homework_page_bloc.dart (1 hunks)
- lib/hausaufgabenheft_logik/test/create_homework_util.dart (3 hunks)
- lib/test_randomness/.gitignore (1 hunks)
- lib/test_randomness/.metadata (1 hunks)
- lib/test_randomness/lib/src/test_randomness.dart (1 hunks)
- lib/test_randomness/lib/test_randomness.dart (1 hunks)
- tools/sz_repo_cli/lib/src/commands/src/test_command.dart (10 hunks)
Files skipped from review due to trivial changes (3)
- lib/test_randomness/.gitignore
- lib/test_randomness/.metadata
- lib/test_randomness/lib/test_randomness.dart
Additional comments: 25
lib/hausaufgabenheft_logik/test/create_homework_util.dart (2)
- 13-13: The import of the
test_randomnesspackage is correctly added to support deterministic testing by generating seed-based random IDs.- 24-24: The usage of
randomAlphaNumeric(5)from thetest_randomnesspackage for generating random IDs aligns with the goal of introducing deterministic testing. Ensure that the seed is properly managed across tests for consistent results.lib/test_randomness/lib/src/test_randomness.dart (1)
- 1-51: The implementation of the
test_randomnesslibrary, including the handling of therandomnessSeed, the creation of theszTestRandomobject, and the methods for generating random values, is correctly done to support deterministic testing. This approach ensures that tests can produce consistent and predictable outcomes when rerun with the same seed.app/test_goldens/blackboard/details/blackboard_item_read_by_users_list_page_test.dart (2)
- 21-21: The import of the
test_randomnesspackage is correctly added to support deterministic testing by generating seed-based random boolean values.- 43-43: Replacing
Random().nextBool()withszTestRandom.nextBool()for generating random boolean values aligns with the goal of deterministic testing. This ensures that the randomness in tests can be controlled and reproduced with the same seed.app/test/timetable/school_class_filter_view_test.dart (1)
- 12-12: The import of the
test_randomnesspackage replacesrandom_string, aligning with the goal of deterministic testing by using seed-based random value generation.app/test_goldens/calendrical_events/page/past_calendrical_events_page_test.dart (2)
- 22-22: The import of the
test_randomnesspackage is correctly added to support deterministic testing by generating seed-based random values.- 37-47: Replacing the usage of
dart:math'sRandomwithszTestRandomfrom thetest_randomnesspackage for generating random values aligns with the goal of deterministic testing. This ensures that the randomness in tests can be controlled and reproduced with the same seed.app/test/feedback/feedback_page_test.dart (1)
- 18-18: The import of the
test_randomnesspackage replacesrandom_string, aligning with the goal of deterministic testing by using seed-based random value generation.app/test/dashboard/update_reminder_bloc_test.dart (2)
- 10-13: Reordering of import statements is a minor change that improves the organization of imports.
- 142-142: Using
szTestRandom.nextInt()for generating a random number within theReleaseclass aligns with the goal of deterministic testing. This ensures that the randomness in tests can be controlled and reproduced with the same seed.tools/sz_repo_cli/lib/src/commands/src/test_command.dart (3)
- 38-46: The addition of the
test-randomize-ordering-seedoption to specify a seed for randomizing test case execution order is a valuable enhancement to the testing framework. This allows for more controlled and deterministic testing scenarios.- 82-83: Correct usage of the
test-randomize-ordering-seedoption in therunTestsfunction to pass the seed to the testing framework enhances deterministic testing capabilities.- 133-140: The implementation in
_runTestsDartto pass thetest-randomize-ordering-seedto the Dart testing command correctly supports deterministic testing by allowing the specification of a seed.app/test/groups/group_join_select_courses_bloc_test.dart (1)
- 21-21: The import change from
random_stringtotest_randomnessis consistent with the PR's objective to introduce determinism into tests. Ensure that all instances of randomness in this test file now utilize thetest_randomnesspackage to maintain deterministic behavior.app/test/holidays/holiday_bloc_unit_test.dart (1)
- 13-13: The import change from
random_stringtotest_randomnessaligns with the PR's goal of deterministic testing. Verify that thetest_randomnesspackage is correctly used throughout the test to ensure deterministic outcomes.app/test/feedback/feedback_bloc_test.dart (1)
- 17-17: The import change from
random_stringtotest_randomnesssupports the PR's aim for deterministic testing. Ensure that thetest_randomnesspackage is utilized correctly in all tests to achieve deterministic outcomes.lib/hausaufgabenheft_logik/lib/src/lehrer/teacher_homework_page_bloc.dart (1)
- 17-17: The import change from
random_stringtotest_randomnessis consistent with the PR's objective to ensure deterministic testing. Confirm that the deterministic approach is correctly applied in functions likerandomHomeworkViewWithto maintain consistency across the project.app/test/timetable/timetable_bloc_test.dart (2)
- 20-20: The addition of the
test_randomnesspackage is aligned with the PR's objective to introduce determinism into tests. This change enables the use of a seed-based approach for generating random numbers, which is crucial for reproducing test scenarios.- 52-52: The replacement of
Random().nextInt()withszTestRandom.nextInt(200).toString()is a key part of making the tests deterministic. This ensures that the randomness in tests can be controlled and reproduced using a seed. Please ensure thatszTestRandomis properly initialized and that its usage here aligns with the intended test scenarios.app/test/homework/teacher/teacher_homework_page_widget_test.dart (2)
- 17-17: The modification to prefix
flutter.Colorwithflutteris a good practice to avoid naming conflicts with otherColorclasses from different packages. This change improves code clarity.- 29-29: The addition of the
test_randomnesspackage is aligned with the PR's objective to introduce determinism into tests. This change enables the use of a seed-based approach for generating random values, which is crucial for reproducing test scenarios. Ensure that thetest_randomnesspackage is used appropriately throughout the tests to replace non-deterministic randomness.lib/abgabe/abgabe_client_lib/test/bloc_test.dart (2)
- 28-28: The import statement for
test_randomnessreplaces the previousrandom_stringpackage, aligning with the PR's objective to introduce determinism into tests. This change is crucial for achieving consistent and predictable test outcomes. Ensure that all instances where randomness is required in tests are now utilizing this new package.- 28-28: While the import change to
test_randomnessis correctly applied, it's important to verify that the functionality provided by this package is properly utilized throughout the test suite. Specifically, look for instances where random values are generated for test purposes and ensure they are now derived from a seeded random generator provided bytest_randomness. This might require a deeper review of the test cases and possibly the implementation of thetest_randomnesspackage itself.app/test/homework/homework_dialog_test.dart (1)
- 28-28: The import statement has been updated from
'package:random_string/random_string.dart'to'package:test_randomness/test_randomness.dart'. This change aligns with the PR's objective to introduce determinism into tests by leveraging a seed-based approach for randomness. Ensure that all instances where randomness is required in this test file (and others affected by this change) are updated to use the newtest_randomnesspackage functionalities appropriately.
| import 'package:mockito/annotations.dart'; | ||
| import 'package:mockito/mockito.dart'; | ||
| import 'package:path/path.dart'; | ||
| import 'package:random_string/random_string.dart'; | ||
| import 'package:test_randomness/test_randomness.dart'; | ||
| import 'package:sharezone/homework/homework_dialog/homework_dialog.dart'; | ||
| import 'package:sharezone/homework/homework_dialog/homework_dialog_bloc.dart'; | ||
| import 'package:sharezone/main/application_bloc.dart'; |
There was a problem hiding this comment.
📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [1-1000]
Overall, the test file appears well-structured and comprehensive, covering a wide range of scenarios for the HomeworkDialog component. However, it's essential to ensure that all randomness in tests, such as generating random IDs, booleans, or other data, now utilizes the test_randomness package's functionalities to maintain determinism. This approach will enhance the reproducibility of test scenarios and facilitate debugging when tests fail. Additionally, consider reviewing the entire test suite for consistency in using the new deterministic randomness approach and ensuring that all tests adhere to best practices for clarity, maintainability, and efficiency.
| @@ -25,7 +25,7 @@ import 'package:common_domain_models/common_domain_models.dart'; | |||
| import 'package:files_basics/files_models.dart'; | |||
| import 'package:files_basics/local_file.dart'; | |||
|
|
|||
| import 'package:random_string/random_string.dart'; | |||
| import 'package:test_randomness/test_randomness.dart'; | |||
There was a problem hiding this comment.
Consider adding documentation or comments explaining the purpose and usage of the test_randomness package within this test suite, especially if there are specific patterns or practices that developers should follow when using this package for deterministic testing. This will help maintain the clarity and maintainability of the test suite.
|
Visit the preview URL for this PR (updated for commit b100ad3): https://sharezone-website-dev--pr1320-sz-test-randomness-f-39gopfye.web.app (expires Sun, 10 Mar 2024 20:33:31 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: 372b0431a96247f908d9a97d5d865de1c8b3b04e |
|
Visit the preview URL for this PR (updated for commit b100ad3): https://sharezone-test--pr1320-sz-test-randomness-f-dondktz3.web.app (expires Sun, 10 Mar 2024 20:33:48 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: 4cb3ae61e1e018abfd9841fd3239f5b49ccc034b |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (2)
- lib/test_randomness/lib/src/test_randomness.dart (1 hunks)
- lib/test_randomness/lib/test_randomness.dart (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- lib/test_randomness/lib/src/test_randomness.dart
- lib/test_randomness/lib/test_randomness.dart
| test_randomness: | ||
| path: ../test_randomness |
There was a problem hiding this comment.
Isn't this dependency only used in /test, so could it be a dev dependency?
There was a problem hiding this comment.
No, because the method randomHomeworkViewWith is inside the normal code for a fake "new" teacher bloc with random states that is still left inside the code. I wouldn't want to remove with this PR, so this usage is still present and thus I need to have it in the normal dependencies.
This PR makes tests that use randomness deterministic. This is done by using the
TEST_RANDOMNESS_SEEDenvironment variable that is passed to Dart by thesz testcommand. This seed will be used for generating randomness, so that we can reuse a seed that made a test fail in e.g. a CI pipeline.I replaced all instances of randomness from
package:random_stringwith a new custom package calledtest_randomness.This package also includes a seeded
RandomcalledszTestRandomthat should be used in all future tests.Requires #1318
Summary by CodeRabbit
New Features
Tests
test_randomnesslibrary for generating random numbers and strings, improving test reliability and predictability.Chores
.gitignorerules for a wide range of development environments, ensuring a cleaner repository.Documentation
test_randomnesslibrary, ensuring better project management and version control.