chore: Add notification condition tests #1385
Conversation
|
Caution Review failedThe pull request is closed. 📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces changes to two primary files. First, it updates the Changes
Possibly related PRs
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 using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (4)
tests/Digdir.Domain.Dialogporten.Application.Integration.Tests/Features/V1/ServiceOwner/NotificationCondition/NotificationConditionTests.cs (4)
16-20: Consider documenting test data combinations.The test data generation creates all possible combinations, but it would be helpful to document which combinations are expected to pass/fail and why. This would make the test intentions clearer and help with maintenance.
Consider adding a comment explaining the test matrix, for example:
public static IEnumerable<object[]> NotificationConditionTestData() => + // Test matrix: + // - expectedSendNotificationValue: true when activity exists and condition is Exists, or when activity doesn't exist and condition is NotExists + // - activityType: all possible dialog activity types + // - conditionType: Exists/NotExists conditions from bool expectedSendNotificationValue in ExpectedSendNotificationsValues from DialogActivityType.Values activityType in Enum.GetValues(typeof(DialogActivityType.Values)) from NotificationConditionType conditionType in Enum.GetValues(typeof(NotificationConditionType)) select new object[] { activityType, conditionType, expectedSendNotificationValue };
23-28: Test name could be more specific.The current test name
SendNotification_Should_Be_True_When_Conditions_Are_Metdoesn't fully reflect that it tests both positive and negative cases.Consider renaming to something like
SendNotification_Should_Match_Expected_Value_Based_On_Conditions.
31-37: Improve switch statement readability.The switch statement's intent could be clearer. It's setting up test conditions based on whether we expect the notification to be sent or not.
Consider extracting the logic to a helper method with a descriptive name:
-switch (conditionType) -{ - case NotificationConditionType.Exists when expectedSendNotificationValue: - case NotificationConditionType.NotExists when !expectedSendNotificationValue: - AddActivityRequirements(createDialogCommand, activityType); - break; -} +if (ShouldAddActivity(conditionType, expectedSendNotificationValue)) +{ + AddActivityRequirements(createDialogCommand, activityType); +} +private static bool ShouldAddActivity(NotificationConditionType conditionType, bool expectedSendNotificationValue) => + (conditionType == NotificationConditionType.Exists && expectedSendNotificationValue) || + (conditionType == NotificationConditionType.NotExists && !expectedSendNotificationValue);
64-76: Consider improving helper method design.The helper method could be more flexible and maintainable.
Consider these improvements:
- Return the modified command instead of modifying it directly
- Make the transmission type configurable
- Add validation for required parameters
-private static void AddActivityRequirements( +private static CreateDialogCommand AddActivityRequirements( CreateDialogCommand createDialogCommand, - DialogActivityType.Values activityType) + DialogActivityType.Values activityType, + DialogTransmissionType.Values transmissionType = DialogTransmissionType.Values.Information) { + ArgumentNullException.ThrowIfNull(createDialogCommand); + var activity = DialogGenerator.GenerateFakeDialogActivity(type: activityType); createDialogCommand.Activities.Add(activity); if (activityType is not DialogActivityType.Values.TransmissionOpened) return; - var transmission = DialogGenerator.GenerateFakeDialogTransmissions(type: DialogTransmissionType.Values.Information)[0]; + var transmission = DialogGenerator.GenerateFakeDialogTransmissions(type: transmissionType)[0]; createDialogCommand.Transmissions.Add(transmission); createDialogCommand.Activities[0].TransmissionId = createDialogCommand.Transmissions[0].Id; + + return createDialogCommand; }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
tests/.editorconfig(1 hunks)tests/Digdir.Domain.Dialogporten.Application.Integration.Tests/Features/V1/ServiceOwner/NotificationCondition/NotificationConditionTests.cs(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- tests/.editorconfig
🔇 Additional comments (1)
tests/Digdir.Domain.Dialogporten.Application.Integration.Tests/Features/V1/ServiceOwner/NotificationCondition/NotificationConditionTests.cs (1)
78-96: LGTM! Well-structured test for the not found scenario.
The test follows best practices:
- Clear naming
- Proper AAA pattern
- Thorough assertions
...tegration.Tests/Features/V1/ServiceOwner/NotificationCondition/NotificationConditionTests.cs
Show resolved
Hide resolved
...tegration.Tests/Features/V1/ServiceOwner/NotificationCondition/NotificationConditionTests.cs
Show resolved
Hide resolved
...tegration.Tests/Features/V1/ServiceOwner/NotificationCondition/NotificationConditionTests.cs
Show resolved
Hide resolved
|
Summary by CodeRabbit
New Features
Chores