ocket8888 commented on code in PR #6762:
URL: https://github.com/apache/trafficcontrol/pull/6762#discussion_r871577902
##########
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:
> ... we know what pre-requisite data we are using through tc-fixtures.json
Yes, but the only way the tests know that is through the `testData` array. I
suppose it doesn't really matter because the test case covers it, so if anyone
changes the data they'll have to fix this hard-coded value too, or it won't
pass. Or, at least, we hope it won't pass, unless there's some kind of strange
bug in TO where it returns a garbled notification that just so happens to fit
this hard-coded value. But that does seem unlikely. I don't understand why
> Using testData.CDNs[0].Name would introduce some confusion...
especially since
> ... having to be *(sic)* track of which specific cdn its *(sic)* referring
to
is what you have to do anyway, but this is just having humans do it instead
of having the computer do it programmatically.
Alternatively, this could just check that a notification of the form
<code>test notification: <var>CDN Name</var></code> 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.
--
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]
