SLVS-1554 CaYC Replace Moq with NSubstitute #5774
Conversation
hashicorp-vault-sonar-prod
bot
changed the title
There was a problem hiding this comment.
The tests look a lot cleaner! I like that I can read them from a glance (although we still have a few that have around 20 lines of code, which is not ideal)
But the helper methods have a lot of optional and out parameters. I think we could improve that by introducing notifications for different cases as fields and then just mock them accordingly in each test (see other feedback)
|
|
||
| private static INotification CreateNotification(string id = null, bool oncePerSession = true, params INotificationAction[] actions) | ||
| private IInfoBar ShowNotification(out INotification notification, string id = null, bool oncePerSession = true, bool disabled = false, params INotificationAction[] actions) |
There was a problem hiding this comment.
Instead of having an out parameter, please add the INotification as a field and use the field instead.
You can have multiple notification fields for different cases:
INotification oncePerSessionNotification
INotification multipleTimesPerSession
INotification notificationWithActions etc
There was a problem hiding this comment.
I've introduced a new record AttachedNotification which hold the INotification and IInfoBar, I believe it helped a lot with readability. No more out var 😁
| } | ||
|
|
||
| private static Mock<IInfoBarManager> CreateInfoBarManager(INotification notification, Mock<IInfoBar> infoBar) | ||
| private void AttachNotification(INotification notification, out IInfoBar infoBar) |
There was a problem hiding this comment.
Same for IInfoBar. Instead of using out parameter use a field. It simplifies the code a lot and makes it cleaner
vnaskos-sonar
force-pushed
the
vn/slvs-1183-cayc
branch
from
bcf1476 to
416f190
Compare
|
SLVS-1554
Part of SLVS-1183