ericholguin commented on code in PR #6762:
URL: https://github.com/apache/trafficcontrol/pull/6762#discussion_r871610384
##########
traffic_ops/testing/api/v4/cdnnotifications_test.go:
##########
@@ -53,26 +80,21 @@ func CreateTestCDNNotifications(t *testing.T) {
var opts client.RequestOptions
for _, cdn := range testData.CDNs {
resp, _, err :=
TOSession.CreateCDNNotification(tc.CDNNotificationRequest{CDN: cdn.Name,
Notification: "test notification: " + cdn.Name}, opts)
Review Comment:
I think the confusion would be that someone would have to verify that the
cdn at that index is the correct one for that specific test. (In this scenario
it does not matter since they all have notifications) But this applies to all
tests were we use a specific object for a specific test. Previously, before the
refactor the index was being hardcoded, and I see that more confusing, since I
would have to count in fixtures to see which object the test was referring to.
> Alternatively, this could just check that a notification of the form test
notification: CDN Name exists for every CDN instead of singling one out. In
fact, I just checked and that's what the old tests used to do, so this is
technically losing coverage by not replicating that.
It is bad testing practice to test the same thing multiple times, which a
lot of tests currently do, we aren't losing coverage by removing duplicate
tests.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
