You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@trafficcontrol.apache.org by GitBox <gi...@apache.org> on 2022/03/30 20:51:08 UTC
[GitHub] [trafficcontrol] TaylorCFrey commented on a change in pull request #6690: Refactor DeliveryService Tests
TaylorCFrey commented on a change in pull request #6690:
URL: https://github.com/apache/trafficcontrol/pull/6690#discussion_r838952223
##########
File path: traffic_ops/testing/api/v4/deliveryservices_test.go
##########
@@ -17,2896 +17,772 @@ package v4
import (
"encoding/json"
- "fmt"
"net/http"
"net/url"
- "reflect"
"strconv"
- "strings"
"testing"
"time"
"github.com/apache/trafficcontrol/lib/go-rfc"
"github.com/apache/trafficcontrol/lib/go-tc"
- "github.com/apache/trafficcontrol/lib/go-util"
- "github.com/apache/trafficcontrol/traffic_ops/traffic_ops_golang/deliveryservice"
+ "github.com/apache/trafficcontrol/traffic_ops/testing/api/assert"
+ "github.com/apache/trafficcontrol/traffic_ops/testing/api/utils"
+ "github.com/apache/trafficcontrol/traffic_ops/toclientlib"
client "github.com/apache/trafficcontrol/traffic_ops/v4-client"
)
func TestDeliveryServices(t *testing.T) {
Review comment:
This is just a general observation and question, but not a requirement for any change.
I don't believe this PR changes how these tests are necessarily run. Even though the previous tests were written as individual tests, they still had a hard requirement and dependency on `WithObjs()`. It appeared like we could have run those tests individually, but with their dependency on the objects being initialized ahead of time, they couldn't actually be done that way.
Since these are now table tests, and the dependency on `WithObjs` is still there, we still don't really have the ability to run these individually, is that correct?
##########
File path: traffic_ops/testing/api/v3/deliveryservices_test.go
##########
@@ -17,1180 +17,499 @@ package v3
import (
"encoding/json"
- "fmt"
"net/http"
"net/url"
- "reflect"
"strconv"
- "strings"
"testing"
"time"
"github.com/apache/trafficcontrol/lib/go-rfc"
"github.com/apache/trafficcontrol/lib/go-tc"
- "github.com/apache/trafficcontrol/lib/go-util"
- toclient "github.com/apache/trafficcontrol/traffic_ops/v3-client"
+ "github.com/apache/trafficcontrol/traffic_ops/testing/api/assert"
+ "github.com/apache/trafficcontrol/traffic_ops/testing/api/utils"
+ "github.com/apache/trafficcontrol/traffic_ops/toclientlib"
)
func TestDeliveryServices(t *testing.T) {
- WithObjs(t, []TCObj{CDNs, Types, Tenants, Users, Parameters, Profiles, Statuses, Divisions, Regions, PhysLocations, CacheGroups, Servers, Topologies, ServerCapabilities, DeliveryServices}, func() {
- currentTime := time.Now().UTC().Add(-5 * time.Second)
- ti := currentTime.Format(time.RFC1123)
- var header http.Header
- header = make(map[string][]string)
- header.Set(rfc.IfModifiedSince, ti)
- header.Set(rfc.IfUnmodifiedSince, ti)
- if includeSystemTests {
- SSLDeliveryServiceCDNUpdateTest(t)
- GetTestDeliveryServicesURLSigKeys(t)
+ WithObjs(t, []TCObj{CDNs, Types, Tenants, Users, Parameters, Profiles, Statuses, Divisions, Regions, PhysLocations, CacheGroups, Servers, Topologies, ServerCapabilities, DeliveryServices, DeliveryServicesRequiredCapabilities, DeliveryServiceServerAssignments}, func() {
+
+ tomorrow := time.Now().AddDate(0, 0, 1).Format(time.RFC1123)
+ currentTime := time.Now().UTC().Add(-15 * time.Second)
+ currentTimeRFC := currentTime.Format(time.RFC1123)
+
+ tenant4UserSession := utils.CreateV3Session(t, Config.TrafficOps.URL, "tenant4user", "pa$$word", Config.Default.Session.TimeoutInSecs)
+
+ methodTests := utils.V3TestCase{
+ "GET": {
+ "NOT MODIFIED when NO CHANGES made": {
+ ClientSession: TOSession, RequestHeaders: http.Header{rfc.IfModifiedSince: {tomorrow}},
+ Expectations: utils.CkRequest(utils.NoError(), utils.HasStatus(http.StatusNotModified)),
+ },
+ "OK when VALID request": {
+ ClientSession: TOSession, Expectations: utils.CkRequest(utils.NoError(), utils.HasStatus(http.StatusOK)),
+ },
+ "OK when ACTIVE=TRUE": {
+ ClientSession: TOSession, RequestParams: url.Values{"active": {"true"}},
+ Expectations: utils.CkRequest(utils.NoError(), utils.HasStatus(http.StatusOK), utils.ResponseLengthGreaterOrEqual(1),
+ validateGetDSExpectedFields(map[string]interface{}{"Active": true})),
+ },
+ "OK when ACTIVE=FALSE": {
+ ClientSession: TOSession, RequestParams: url.Values{"active": {"false"}},
+ Expectations: utils.CkRequest(utils.NoError(), utils.HasStatus(http.StatusOK), utils.ResponseLengthGreaterOrEqual(1),
+ validateGetDSExpectedFields(map[string]interface{}{"Active": false})),
+ },
+ "OK when VALID ACCESSIBLETO parameter": {
+ ClientSession: TOSession, RequestParams: url.Values{"accessibleTo": {"1"}},
+ Expectations: utils.CkRequest(utils.NoError(), utils.HasStatus(http.StatusOK), utils.ResponseLengthGreaterOrEqual(1)),
+ },
+ "EMPTY RESPONSE when TENANT attempts reading DS OUTSIDE TENANCY": {
+ ClientSession: tenant4UserSession, RequestParams: url.Values{"xmlId": {"ds3"}},
+ Expectations: utils.CkRequest(utils.NoError(), utils.HasStatus(http.StatusOK), utils.ResponseHasLength(0)),
+ },
+ },
+ "POST": {
Review comment:
This applies to both v3 and v4 test suites:
Does the order of these tests matter? Especially for endpoints that aren't idempotent (`CUD`, but not `R`)? Granted it wouldn't necessarily be prudent to call `DELETE` randomly, but I mean does the order matter in, say, all the `POST` or `PUT` requests?
##########
File path: traffic_ops/testing/api/v3/deliveryservices_test.go
##########
@@ -17,1180 +17,499 @@ package v3
import (
"encoding/json"
- "fmt"
"net/http"
"net/url"
- "reflect"
"strconv"
- "strings"
"testing"
"time"
"github.com/apache/trafficcontrol/lib/go-rfc"
"github.com/apache/trafficcontrol/lib/go-tc"
- "github.com/apache/trafficcontrol/lib/go-util"
- toclient "github.com/apache/trafficcontrol/traffic_ops/v3-client"
+ "github.com/apache/trafficcontrol/traffic_ops/testing/api/assert"
+ "github.com/apache/trafficcontrol/traffic_ops/testing/api/utils"
+ "github.com/apache/trafficcontrol/traffic_ops/toclientlib"
)
func TestDeliveryServices(t *testing.T) {
- WithObjs(t, []TCObj{CDNs, Types, Tenants, Users, Parameters, Profiles, Statuses, Divisions, Regions, PhysLocations, CacheGroups, Servers, Topologies, ServerCapabilities, DeliveryServices}, func() {
- currentTime := time.Now().UTC().Add(-5 * time.Second)
- ti := currentTime.Format(time.RFC1123)
- var header http.Header
- header = make(map[string][]string)
- header.Set(rfc.IfModifiedSince, ti)
- header.Set(rfc.IfUnmodifiedSince, ti)
- if includeSystemTests {
- SSLDeliveryServiceCDNUpdateTest(t)
- GetTestDeliveryServicesURLSigKeys(t)
+ WithObjs(t, []TCObj{CDNs, Types, Tenants, Users, Parameters, Profiles, Statuses, Divisions, Regions, PhysLocations, CacheGroups, Servers, Topologies, ServerCapabilities, DeliveryServices, DeliveryServicesRequiredCapabilities, DeliveryServiceServerAssignments}, func() {
+
+ tomorrow := time.Now().AddDate(0, 0, 1).Format(time.RFC1123)
+ currentTime := time.Now().UTC().Add(-15 * time.Second)
+ currentTimeRFC := currentTime.Format(time.RFC1123)
+
+ tenant4UserSession := utils.CreateV3Session(t, Config.TrafficOps.URL, "tenant4user", "pa$$word", Config.Default.Session.TimeoutInSecs)
+
+ methodTests := utils.V3TestCase{
+ "GET": {
Review comment:
It might behove us to make these HTTP verbs and switch strings `const` somewhere
```go
const (
Get = "GET"
Post = "POST"
GetAfterChanges = "GET AFTER CHANGES"
// ...
)
```
Go's capitalization results in scope changes so the convention of ALL CAPS may not be appropriate, but these could be put somewhere so they are accessible everywhere? I will leave that decision up to you. Maybe the HTTP verbs could be someone globally accessible, but the delivery service specific (`DELIVERY SERVICES CAPACITY`) could be local?
I won't put this comment on every verb, but it applies for `GET` `POST` `PUT` `DELETE` etc.
##########
File path: traffic_ops/testing/api/v3/deliveryservices_test.go
##########
@@ -17,1180 +17,490 @@ package v3
import (
"encoding/json"
- "fmt"
"net/http"
"net/url"
- "reflect"
"strconv"
- "strings"
"testing"
"time"
"github.com/apache/trafficcontrol/lib/go-rfc"
"github.com/apache/trafficcontrol/lib/go-tc"
- "github.com/apache/trafficcontrol/lib/go-util"
- toclient "github.com/apache/trafficcontrol/traffic_ops/v3-client"
+ "github.com/apache/trafficcontrol/traffic_ops/testing/api/assert"
+ "github.com/apache/trafficcontrol/traffic_ops/testing/api/utils"
+ "github.com/apache/trafficcontrol/traffic_ops/toclientlib"
)
func TestDeliveryServices(t *testing.T) {
- WithObjs(t, []TCObj{CDNs, Types, Tenants, Users, Parameters, Profiles, Statuses, Divisions, Regions, PhysLocations, CacheGroups, Servers, Topologies, ServerCapabilities, DeliveryServices}, func() {
- currentTime := time.Now().UTC().Add(-5 * time.Second)
- ti := currentTime.Format(time.RFC1123)
- var header http.Header
- header = make(map[string][]string)
- header.Set(rfc.IfModifiedSince, ti)
- header.Set(rfc.IfUnmodifiedSince, ti)
- if includeSystemTests {
- SSLDeliveryServiceCDNUpdateTest(t)
- GetTestDeliveryServicesURLSigKeys(t)
+ WithObjs(t, []TCObj{CDNs, Types, Tenants, Users, Parameters, Profiles, Statuses, Divisions, Regions, PhysLocations, CacheGroups, Servers, Topologies, ServerCapabilities, DeliveryServices, DeliveryServicesRequiredCapabilities, DeliveryServiceServerAssignments}, func() {
+
+ tomorrow := time.Now().AddDate(0, 0, 1).Format(time.RFC1123)
+ currentTime := time.Now().UTC().Add(-15 * time.Second)
+ currentTimeRFC := currentTime.Format(time.RFC1123)
Review comment:
In general, when I'm writing tests that deal with time.Now() I try to grab it once, then reuse it to ensure it doesn't actually change on me with how fast some processing can happen. Calls that are immediately executed could still have different _now_ times. So I'll do something like:
```go
currentTime := time.Now().UTC()
currentTimeRFC := currentTime.Format(time.RFC1123)
tomorrow := currentTime.AddDate(0,0,1).Format(time.RFC1123) // Ensures there aren't nanoseconds worth of time that throws off the +1 day or equality later on, if need be
...
```
--
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: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org