ericholguin commented on code in PR #6762:
URL: https://github.com/apache/trafficcontrol/pull/6762#discussion_r871691689
##########
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:
> That's not quite a duplicate test. If I change the notifications code so
that no matter what you POST to it, it just stores the static string test
notification: cdn2 it would have failed the old tests, but will pass the tests
as of this rewrite
If someone is hardcoding core code to pass tests and that slips by a review,
that falls outside the scope of what a test can cover.
> or if I change it to only store the first notification it gets and use it
as the text content of all future notifications, or if a database change is
introduced with a bad constraint such that it will store one notification just
fine but any and all future ones erroneously violate a constraint and TO
doesn't handle the resulting db error properly and just returns a 200. So
there's a case not being covered anymore. Those are all pretty unlikely
scenarios, but they used to be tested against and won't be anymore.
Those are technically covered by this test, the test performs a `GET`
request against the second cdn notification `cdn2` not `cdn1` which is the
first notification created. But again really unlikely scenarios where we wonder
what actual value they provide.
> It probably suffices to check two before the tests become truly
degenerate, but as it's a read-only operation I wouldn't see the harm in doing
them all.
This is a common discussion when it comes to testing, where there isn't a
good answer. Just because we can test everything doesn't mean we should. Those
scenarios are rare and this endpoint isn't crucial. Although adding those tests
wouldn't bring any harm, we are only increasing the amount of tests and test
time but with not much of a gain.
> On the other hand, since nothing actually depends on CDN notifications,
and they're never used in a WithObjs anywhere, it may not be best to have them
in the testing fixture data at all, and the tests could just make a couple with
some randomly generated strings and then verify that the notifications are in
fact created with that text. Random strings in tests do usually make me
uncomfortable, but I can't really justify that feeling with any argument.
Actually notifications themselves are not in the fixtures data, it's using
the cdns that are defined in fixtures to create the notification.
I'm okay with adding two constant values that the prerequisite Create
function will create, so no longer leverage testData.CDN, the cdnName will
still be hardcoded against what exists in fixtures. And also adding a test for
both - one for the valid request and for checking that the notification varies
if you agree to 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]
