ocket8888 commented on a change in pull request #5717:
URL: https://github.com/apache/trafficcontrol/pull/5717#discussion_r609890309
##########
File path: traffic_ops/testing/api/v4/deliveryservices_test.go
##########
@@ -1238,6 +1254,272 @@ func VerifyPaginationSupportDS(t *testing.T) {
}
}
+func GetDeliveryServiceByCdn(t *testing.T) {
+
+ if len(testData.DeliveryServices) > 0 {
+ firstDS := testData.DeliveryServices[0]
+
+ if firstDS.CDNID == nil && firstDS.CDNName != nil {
+ cdns, _, err :=
TOSession.GetCDNByNameWithHdr(*firstDS.CDNName, nil)
+ if err != nil {
+ t.Fatalf("Error in Getting CDN by Name: %v",
err)
+ }
+ if len(cdns) == 0 {
+ t.Fatalf("no CDN named %v" + *firstDS.CDNName)
+ }
+ firstDS.CDNID = &cdns[0].ID
+ }
+ resp, _, err :=
TOSession.GetDeliveryServicesByCDNIDWithHdr(*firstDS.CDNID, nil)
Review comment:
all of the tests added already guard against a few segfaults with stuff
like `if len(testData.DeliveryServices) > 0` and `if firstDS.CDNID == nil &&
firstDS.CDNName != nil` so I figured he was good on it. That's not really the
issue here. There's just an edge case, because above it checks if the CDNID is
`nil` to avoid a segfault by just fetching the CDN ID from its name, but it
doesn't handle the case where the name is also `nil`. If it were me, I'd just
write this like:
```go
func GetDeliveryServiceByCdn(t *testing.T) {
if len(testData.DeliveryServices) < 1 {
t.Fatal("Need at least one Delivery Service to test getting
Delivery Services by CDN")
}
firstDS := testData.DeliveryServices[0]
if firstDS.CDNID == nil {
if firstDS.CDNName == nil {
t.Fatal("Cannot get CDN ID for test Delivery Service:
CDN Name was null or undefined")
}
// Get CDNID from the CDN Name and assign a reference to it to
firstDS.CDNID
}
// Continue with the test}
```
but basically there're two ways to check for `nil` values/array length to
avoid segmentation faults. You can do it the way you're already doing it where
you only do things if it's not `nil`, so like
```go
func TestSomething(t *testing.T) {
value := funcThatPossiblyReturnsNil()
if value != nil {
// the rest of the test
}
}
```
but personally I don't like that approach because it can cause some pretty
deep nesting. Instead, I like to consider not having the right data to test a
test failure itself, so I'd write that test like:
```go
func TestSomething(t *testing.T) {
value := funcThatPossiblyReturnsNil()
if value == nil {
t.Fatal("testing value was nil")
}
// the rest of the test
}
```
```go
--
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.
For queries about this service, please contact Infrastructure at:
[email protected]
