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/04/15 18:16:48 UTC

[GitHub] [trafficcontrol] ericholguin opened a new pull request, #6758: Refactor Delivery Service Requests

ericholguin opened a new pull request, #6758:
URL: https://github.com/apache/trafficcontrol/pull/6758

   <!--
   Thank you for contributing! Please be sure to read our contribution guidelines: https://github.com/apache/trafficcontrol/blob/master/CONTRIBUTING.md
   If this closes or relates to an existing issue, please reference it using one of the following:
   
   Closes: #ISSUE
   Related: #ISSUE
   
   If this PR fixes a security vulnerability, DO NOT submit! Instead, contact
   the Apache Traffic Control Security Team at security@trafficcontrol.apache.org and follow the
   guidelines at https://apache.org/security regarding vulnerability disclosure.
   -->
   
   **DEPENDS ON PR: https://github.com/apache/trafficcontrol/pull/6746** 
   
   This PR refactors **v4/deliveryservice_requests_test.go**, **v3/deliveryservice_requests_test.go**, in order to make tests more granular, providing transparency on what is being tested and reducing the need to dig through test code to confirm what was tested. In addition this new test structure should also make it easier for new tests to be added as well as reduce redundant code.
   
   - Adds data driven testing
       - Descriptive test names
   - Sub tests for organization
       - Use GoLangs test runner
   - Use assert functionality
       - Provides fundamental assertions
       - Streamlines code
       - Reduces nested conditionals
   - Adds expectation functions
        - Test specific expectations in a clear concise way
        - Reusable expectations
   
   
   <!-- **^ Add meaningful description above** --><hr/>
   
   ## Which Traffic Control components are affected by this PR?
   <!-- Please delete all components from this list that are NOT affected by this PR.
   Feel free to add the name of a tool or script that is affected but not on the list.
   -->
   
   - Traffic Ops Testing
   
   ## What is the best way to verify this PR?
   <!-- Please include here ALL the steps necessary to test your PR.
   If your PR has tests (and most should), provide the steps needed to run the tests.
   If not, please provide step-by-step instructions to test the PR manually and explain why your PR does not need tests. -->
   
   Run TO Integration Tests
   
   
   ## PR submission checklist
   - [x] This PR has tests <!-- If not, please delete this text and explain why this PR does not need tests. -->
   - [x] This PR **DOES NOT FIX A SERIOUS SECURITY VULNERABILITY** (see [the Apache Software Foundation's security guidelines](https://apache.org/security) for details)
   
   <!--
   Licensed to the Apache Software Foundation (ASF) under one
   or more contributor license agreements.  See the NOTICE file
   distributed with this work for additional information
   regarding copyright ownership.  The ASF licenses this file
   to you under the Apache License, Version 2.0 (the
   "License"); you may not use this file except in compliance
   with the License.  You may obtain a copy of the License at
   
       http://www.apache.org/licenses/LICENSE-2.0
   
   Unless required by applicable law or agreed to in writing,
   software distributed under the License is distributed on an
   "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
   KIND, either express or implied.  See the License for the
   specific language governing permissions and limitations
   under the License.
   -->
   


-- 
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


[GitHub] [trafficcontrol] zrhoffman merged pull request #6758: Refactor Delivery Service Requests

Posted by GitBox <gi...@apache.org>.
zrhoffman merged PR #6758:
URL: https://github.com/apache/trafficcontrol/pull/6758


-- 
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


[GitHub] [trafficcontrol] rimashah25 commented on a diff in pull request #6758: Refactor Delivery Service Requests

Posted by GitBox <gi...@apache.org>.
rimashah25 commented on code in PR #6758:
URL: https://github.com/apache/trafficcontrol/pull/6758#discussion_r861946107


##########
traffic_ops/testing/api/v4/deliveryservice_requests_test.go:
##########
@@ -16,672 +16,356 @@ package v4
 */
 
 import (
+	"encoding/json"
 	"net/http"
+	"net/url"
 	"strconv"
 	"strings"
 	"testing"
 	"time"
 
 	"github.com/apache/trafficcontrol/lib/go-rfc"
-	tc "github.com/apache/trafficcontrol/lib/go-tc"
-	"github.com/apache/trafficcontrol/lib/go-util"
+	"github.com/apache/trafficcontrol/lib/go-tc"
+	"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"
 )
 
-const (
-	dsrGood      = 0
-	dsrBadTenant = 1
-	dsrRequired  = 2
-	dsrDraft     = 3
-)
-
-// this resets the IDs of things attached to a DS, which needs to be done
-// because the WithObjs flow destroys and recreates those object IDs
-// non-deterministically with each test - BUT, the client method permanently
-// alters the DSR structures by adding these referential IDs. Older clients
-// got away with it by not making 'DeliveryService' a pointer, but to add
-// original/requested fields you need to sometimes allow each to be nil, so
-// this is a problem that needs to be solved at some point.
-// A better solution _might_ be to reload all the test fixtures every time
-// to wipe any and all referential modifications made to any test data, but
-// for now that's overkill.
-func resetDS(ds *tc.DeliveryServiceV4) {
-	if ds == nil {
-		return
-	}
-	ds.CDNID = nil
-	ds.ID = nil
-	ds.ProfileID = nil
-	ds.TenantID = nil
-	ds.TypeID = nil
-}
-
 func TestDeliveryServiceRequests(t *testing.T) {
 	WithObjs(t, []TCObj{CDNs, Types, Parameters, Tenants, DeliveryServiceRequests}, func() {
-		GetTestDeliveryServiceRequestsIMS(t)
-		GetTestDeliveryServiceRequests(t)
-		currentTime := time.Now().UTC().Add(-5 * time.Second)
-		time := currentTime.Format(time.RFC1123)
-		var header http.Header
-		header = make(map[string][]string)
-		header.Set(rfc.IfModifiedSince, time)
-		header.Set(rfc.IfUnmodifiedSince, time)
-		UpdateTestDeliveryServiceRequestsWithLongDescFields(t)
-		UpdateTestDeliveryServiceRequests(t)
-		UpdateTestDeliveryServiceRequestsWithHeaders(t, header)
-		GetTestDeliveryServiceRequestsIMSAfterChange(t, header)
-		header = make(map[string][]string)
-		etag := rfc.ETag(currentTime)
-		header.Set(rfc.IfMatch, etag)
-		UpdateTestDeliveryServiceRequestsWithHeaders(t, header)
-	})
-}
-
-func UpdateTestDeliveryServiceRequestsWithLongDescFields(t *testing.T) {
-	if len(testData.DeliveryServiceRequests) < dsrGood+1 {
-		t.Fatalf("Need at least %d Delivery Service Requests to test updating them", dsrGood+1)
-	}
-
-	// Retrieve the DeliveryServiceRequest by name so we can get the id for the Update
-	dsr := testData.DeliveryServiceRequests[dsrGood]
-	var ds *tc.DeliveryServiceV4
-	if dsr.ChangeType == tc.DSRChangeTypeDelete {
-		dsr.Original.LongDesc1 = util.StrPtr("long desc 1")
-		dsr.Original.LongDesc2 = util.StrPtr("long desc 2")
-		ds = dsr.Original
-	} else {
-		dsr.Requested.LongDesc1 = util.StrPtr("long desc 1")
-		dsr.Requested.LongDesc2 = util.StrPtr("long desc 2")
-		ds = dsr.Requested
-	}
-
-	if ds == nil || ds.XMLID == nil {
-		t.Fatalf("the %dth DSR in the test data had no DeliveryService - or that DeliveryService had no XMLID", dsrGood)
-	}
-
-	opts := client.NewRequestOptions()
-	opts.QueryParameters.Set("xmlId", *ds.XMLID)
-	resp, _, err := TOSession.GetDeliveryServiceRequests(opts)
-	if err != nil {
-		t.Errorf("cannot get Delivery Service Request with XMLID '%s': %v - alerts: %+v", *ds.XMLID, err, resp.Alerts)
-	}
-	if len(resp.Response) == 0 {
-		t.Fatalf("Expected at least one Deliver Service Request to exist with XMLID '%s', but none were found in Traffic Ops", *ds.XMLID)
-	}
-	respDSR := resp.Response[0]
-	if respDSR.ID == nil {
-		t.Fatalf("got a DSR by XMLID '%s' with a null or undefined ID", *ds.XMLID)
-	}
-	var respDS *tc.DeliveryServiceV4
-	if dsr.ChangeType == tc.DSRChangeTypeDelete {
-		respDS = dsr.Original
-		respDSR.Original = respDS
-	} else {
-		respDS = dsr.Requested
-		respDSR.Requested = respDS
-	}
-	expDisplayName := "new display name"
-	respDS.DisplayName = &expDisplayName
-	id := *respDSR.ID
-	_, reqInf, err := TOSession.UpdateDeliveryServiceRequest(id, respDSR, client.RequestOptions{})
-	if err == nil {
-		t.Errorf("expected an error stating that Long Desc 1 and Long Desc 2 fields are not supported in api version 4.0 onwards, but got nothing")
-	}
-	if reqInf.StatusCode != http.StatusBadRequest {
-		t.Errorf("expected a 400 status code, but got %d", reqInf.StatusCode)
-	}
-}
-
-// Note that this test is susceptible to breaking if the structure of the test
-// data's DSRs is altered at all.
-func UpdateTestDeliveryServiceRequestsWithHeaders(t *testing.T, header http.Header) {
-	if len(testData.DeliveryServiceRequests) < dsrGood+1 {
-		t.Fatalf("Need at least %d Delivery Service Requests to test updating them using headers", dsrGood+1)
-	}
-	// Retrieve the DeliveryServiceRequest by name so we can get the id for the Update
-	dsr := testData.DeliveryServiceRequests[dsrGood]
-	var ds *tc.DeliveryServiceV4
-	if dsr.ChangeType == tc.DSRChangeTypeDelete {
-		ds = dsr.Original
-	} else {
-		ds = dsr.Requested
-	}
-	resetDS(ds)
-	if ds == nil || ds.XMLID == nil {
-		t.Fatalf("the %dth DSR in the test data had no Delivery Service, or that Delivery Service had a null or undefined XMLID", dsrGood)
-	}
-	opts := client.NewRequestOptions()
-	opts.Header = header
-	opts.QueryParameters.Set("xmlId", *ds.XMLID)
-	resp, _, err := TOSession.GetDeliveryServiceRequests(opts)
-	if err != nil {
-		t.Errorf("cannot get Delivery Service Request by XMLID '%s': %v - alerts: %+v", *ds.XMLID, err, resp.Alerts)
-	}
-	if len(resp.Response) == 0 {
-		t.Fatal("Length of GET DeliveryServiceRequest is 0")
-	}
-	respDSR := resp.Response[0]
-	if respDSR.ID == nil {
-		t.Fatalf("Got a DSR for XML ID '%s' that had a nil ID", *ds.XMLID)
-	}
-	if respDSR.ChangeType != dsr.ChangeType {
-		t.Fatalf("remote representation of DSR with XMLID '%s' differed from stored data", *ds.XMLID)
-	}
-	var respDS *tc.DeliveryServiceV4
-	if respDSR.ChangeType == tc.DSRChangeTypeDelete {
-		respDS = respDSR.Original
-	} else {
-		respDS = respDSR.Requested
-	}
-
-	respDS.DisplayName = new(string)
-	*respDS.DisplayName = "new display name"
-	opts.QueryParameters.Del("xmlId")
-	_, reqInf, err := TOSession.UpdateDeliveryServiceRequest(*respDSR.ID, respDSR, opts)
-	if err == nil {
-		t.Errorf("Expected error about precondition failed, but got none")
-	}
-	if reqInf.StatusCode != http.StatusPreconditionFailed {
-		t.Errorf("Expected status code 412, got %v", reqInf.StatusCode)
-	}
-}
-
-// Note that this test is susceptible to breaking if the structure of the test
-// data's DSRs is altered at all.
-func GetTestDeliveryServiceRequestsIMSAfterChange(t *testing.T, header http.Header) {
-	if len(testData.DeliveryServiceRequests) < dsrGood+1 {
-		t.Fatalf("Need at least %d Delivery Service Requests to test updating them with IMS", dsrGood+1)
-	}
-	dsr := testData.DeliveryServiceRequests[dsrGood]
-	var ds *tc.DeliveryServiceV4
-	if dsr.ChangeType == tc.DSRChangeTypeDelete {
-		ds = dsr.Original
-	} else {
-		ds = dsr.Requested
-	}
-
-	resetDS(ds)
-	if ds == nil || ds.XMLID == nil {
-		t.Fatalf("the %dth DSR in the test data had no Delivery Service, or that Delivery Service had a null or undefined XMLID", dsrGood)
-	}
-
-	opts := client.NewRequestOptions()
-	opts.Header = header
-	opts.QueryParameters.Set("xmlId", *ds.XMLID)
-	resp, reqInf, err := TOSession.GetDeliveryServiceRequests(opts)
-	if err != nil {
-		t.Fatalf("Expected no error, but got: %v - alerts: %+v", err, resp.Alerts)
-	}
-	if reqInf.StatusCode != http.StatusOK {
-		t.Fatalf("Expected 200 status code, got %v", reqInf.StatusCode)
-	}
-	currentTime := time.Now().UTC()
-	currentTime = currentTime.Add(1 * time.Second)
-	timeStr := currentTime.Format(time.RFC1123)
-	opts.Header.Set(rfc.IfModifiedSince, timeStr)
-	resp, reqInf, err = TOSession.GetDeliveryServiceRequests(opts)
-	if err != nil {
-		t.Fatalf("Expected no error, but got: %v - alerts: %+v", err, resp.Alerts)
-	}
-	if reqInf.StatusCode != http.StatusNotModified {
-		t.Fatalf("Expected 304 status code, got %v", reqInf.StatusCode)
-	}
-}
 
-// Note that this test is susceptible to breaking if the structure of the test
-// data's DSRs is altered at all.
-func CreateTestDeliveryServiceRequests(t *testing.T) {
-	if len(testData.DeliveryServiceRequests) < dsrGood+1 {
-		t.Fatalf("Need at least %d Delivery Service Requests to test creating Delivery Service Requests", dsrGood+1)
-	}
-	dsr := testData.DeliveryServiceRequests[dsrGood]
-	resetDS(dsr.Original)
-	resetDS(dsr.Requested)
-	respDSR, _, err := TOSession.CreateDeliveryServiceRequest(dsr, client.RequestOptions{})
-	if err != nil {
-		t.Errorf("could not create Delivery Service Requests: %v - alerts: %+v", err, respDSR.Alerts)
-	}
-
-}
-
-// Note that this test is susceptible to breaking if the structure of the test
-// data's DSRs is altered at all.
-func TestDeliveryServiceRequestRequired(t *testing.T) {
-	WithObjs(t, []TCObj{CDNs, Types, Parameters, Tenants}, func() {
-		if len(testData.DeliveryServiceRequests) < dsrRequired+1 {
-			t.Fatalf("Need at least %d Delivery Service Requests to test creating a Delivery Service Request missing required fields", dsrRequired+1)
+		currentTime := time.Now().UTC().Add(-15 * time.Second)
+		currentTimeRFC := currentTime.Format(time.RFC1123)
+		tomorrow := currentTime.AddDate(0, 0, 1).Format(time.RFC1123)
+
+		methodTests := utils.V4TestCase{
+			"GET": {
+				"NOT MODIFIED when NO CHANGES made": {
+					ClientSession: TOSession,
+					RequestOpts:   client.RequestOptions{Header: http.Header{rfc.IfModifiedSince: {tomorrow}}},
+					Expectations:  utils.CkRequest(utils.NoError(), utils.HasStatus(http.StatusNotModified)),
+				},
+				"OK when VALID XMLID parameter": {
+					ClientSession: TOSession,
+					RequestOpts:   client.RequestOptions{QueryParameters: url.Values{"xmlId": {"test-ds1"}}},
+					Expectations: utils.CkRequest(utils.NoError(), utils.HasStatus(http.StatusOK), utils.ResponseLengthGreaterOrEqual(1),
+						validateGetDSRequestFields(map[string]interface{}{"XMLID": "test-ds1"})),
+				},
+			},
+			"PUT": {
+				"OK when VALID request": {
+					EndpointId:    GetDeliveryServiceRequestId(t, "test-ds1"),
+					ClientSession: TOSession,
+					RequestBody: map[string]interface{}{
+						"changeType": "create",

Review Comment:
   Per @ericholguin:
   `Oh yeah so in that scenario the change type can be anything, we’re testing that we can update an existing delivery service request, not testing a delivery service request thats requesting to update a delivery service. If that makes sense`
   
   Rima: confusing but I understand.



-- 
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


[GitHub] [trafficcontrol] rimashah25 commented on a diff in pull request #6758: Refactor Delivery Service Requests

Posted by GitBox <gi...@apache.org>.
rimashah25 commented on code in PR #6758:
URL: https://github.com/apache/trafficcontrol/pull/6758#discussion_r856543542


##########
traffic_ops/testing/api/v3/deliveryservice_requests_test.go:
##########
@@ -16,366 +16,317 @@ package v3
 */
 
 import (
+	"encoding/json"
 	"net/http"
+	"net/url"
 	"strings"
 	"testing"
 	"time"
 
 	"github.com/apache/trafficcontrol/lib/go-rfc"
-	tc "github.com/apache/trafficcontrol/lib/go-tc"
-)
-
-const (
-	dsrGood      = 0
-	dsrBadTenant = 1
-	dsrRequired  = 2
-	dsrDraft     = 3
+	"github.com/apache/trafficcontrol/lib/go-tc"
+	"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 TestDeliveryServiceRequests(t *testing.T) {
 	WithObjs(t, []TCObj{CDNs, Types, Parameters, Tenants, DeliveryServiceRequests}, func() {
-		GetTestDeliveryServiceRequestsIMS(t)
-		GetTestDeliveryServiceRequests(t)
-		currentTime := time.Now().UTC().Add(-5 * time.Second)
-		time := currentTime.Format(time.RFC1123)
-		var header http.Header
-		header = make(map[string][]string)
-		header.Set(rfc.IfModifiedSince, time)
-		header.Set(rfc.IfUnmodifiedSince, time)
-		UpdateTestDeliveryServiceRequests(t)
-		UpdateTestDeliveryServiceRequestsWithHeaders(t, header)
-		GetTestDeliveryServiceRequestsIMSAfterChange(t, header)
-		header = make(map[string][]string)
-		etag := rfc.ETag(currentTime)
-		header.Set(rfc.IfMatch, etag)
-		UpdateTestDeliveryServiceRequestsWithHeaders(t, header)
-	})
-}
-
-func UpdateTestDeliveryServiceRequestsWithHeaders(t *testing.T, header http.Header) {
-	// Retrieve the DeliveryServiceRequest by name so we can get the id for the Update
-	dsr := testData.DeliveryServiceRequests[dsrGood]
-	resp, _, err := TOSession.GetDeliveryServiceRequestByXMLIDWithHdr(dsr.DeliveryService.XMLID, header)
-	if err != nil {
-		t.Errorf("cannot GET DeliveryServiceRequest by name: %v - %v", dsr.DeliveryService.XMLID, err)
-	}
-	if len(resp) == 0 {
-		t.Fatal("Length of GET DeliveryServiceRequest is 0")
-	}
-	respDSR := resp[0]
-	respDSR.DeliveryService.DisplayName = "new display name"
 
-	_, reqInf, err := TOSession.UpdateDeliveryServiceRequestByIDWithHdr(respDSR.ID, respDSR, header)
-	if err == nil {
-		t.Errorf("Expected error about precondition failed, but got none")
-	}
-	if reqInf.StatusCode != http.StatusPreconditionFailed {
-		t.Errorf("Expected status code 412, got %v", reqInf.StatusCode)
-	}
-}
-
-func GetTestDeliveryServiceRequestsIMSAfterChange(t *testing.T, header http.Header) {
-	dsr := testData.DeliveryServiceRequests[dsrGood]
-	_, reqInf, err := TOSession.GetDeliveryServiceRequestByXMLIDWithHdr(dsr.DeliveryService.XMLID, header)
-	if err != nil {
-		t.Fatalf("Expected no error, but got %v", err.Error())
-	}
-	if reqInf.StatusCode != http.StatusOK {
-		t.Fatalf("Expected 200 status code, got %v", reqInf.StatusCode)
-	}
-	currentTime := time.Now().UTC()
-	currentTime = currentTime.Add(1 * time.Second)
-	timeStr := currentTime.Format(time.RFC1123)
-	header.Set(rfc.IfModifiedSince, timeStr)
-	_, reqInf, err = TOSession.GetDeliveryServiceRequestByXMLIDWithHdr(dsr.DeliveryService.XMLID, header)
-	if err != nil {
-		t.Fatalf("Expected no error, but got %v", err.Error())
-	}
-	if reqInf.StatusCode != http.StatusNotModified {
-		t.Fatalf("Expected 304 status code, got %v", reqInf.StatusCode)
-	}
-}
-
-func CreateTestDeliveryServiceRequests(t *testing.T) {
-	t.Log("CreateTestDeliveryServiceRequests")
-
-	dsr := testData.DeliveryServiceRequests[dsrGood]
-	respDSR, _, err := TOSession.CreateDeliveryServiceRequest(dsr)
-	t.Log("Response: ", respDSR)
-	if err != nil {
-		t.Errorf("could not CREATE DeliveryServiceRequests: %v", err)
-	}
-
-}
-
-func TestDeliveryServiceRequestRequired(t *testing.T) {
-	WithObjs(t, []TCObj{CDNs, Types, Parameters, Tenants}, func() {
-		dsr := testData.DeliveryServiceRequests[dsrRequired]
-		_, _, err := TOSession.CreateDeliveryServiceRequest(dsr)
-		if err == nil {
-			t.Error("expected: validation error, actual: nil")
+		currentTime := time.Now().UTC().Add(-15 * time.Second)
+		currentTimeRFC := currentTime.Format(time.RFC1123)
+		tomorrow := currentTime.AddDate(0, 0, 1).Format(time.RFC1123)
+
+		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 XMLID parameter": {
+					ClientSession: TOSession, RequestParams: url.Values{"xmlId": {"test-ds1"}},
+					Expectations: utils.CkRequest(utils.NoError(), utils.HasStatus(http.StatusOK), utils.ResponseLengthGreaterOrEqual(1),
+						validateGetDSRequestFields(map[string]interface{}{"XMLID": "test-ds1"})),
+				},
+			},
+			"PUT": {
+				"OK when VALID request": {
+					EndpointId: GetDeliveryServiceRequestId(t, "test-ds1"), ClientSession: TOSession,
+					RequestBody: map[string]interface{}{
+						"changeType": "create",
+						"deliveryService": generateDeliveryService(t, map[string]interface{}{
+							"displayName": "NEW DISPLAY NAME",
+							"tenantId":    GetTenantId(t, "tenant1"),
+							"xmlId":       "test-ds1",
+						}),
+						"status": "draft",
+					},
+					Expectations: utils.CkRequest(utils.NoError(), utils.HasStatus(http.StatusOK)),
+				},
+				"OK when UPDATING STATUS FROM DRAFT TO SUBMITTED": {
+					EndpointId: GetDeliveryServiceRequestId(t, "test-ds1"), ClientSession: TOSession,
+					RequestBody: map[string]interface{}{
+						"changeType": "create",
+						"deliveryService": generateDeliveryService(t, map[string]interface{}{
+							"tenantId": GetTenantId(t, "tenant1"),
+							"xmlId":    "test-ds1",
+						}),
+						"status": "submitted",
+					},
+					Expectations: utils.CkRequest(utils.NoError(), utils.HasStatus(http.StatusOK)),
+				},
+				"PRECONDITION FAILED when updating with IF-UNMODIFIED-SINCE Header": {
+					EndpointId: GetDeliveryServiceRequestId(t, "test-ds1"), ClientSession: TOSession,
+					RequestHeaders: http.Header{rfc.IfUnmodifiedSince: {currentTimeRFC}},
+					RequestBody:    map[string]interface{}{},
+					Expectations:   utils.CkRequest(utils.HasError(), utils.HasStatus(http.StatusPreconditionFailed)),
+				},
+				"PRECONDITION FAILED when updating with IFMATCH ETAG Header": {
+					EndpointId: GetDeliveryServiceRequestId(t, "test-ds1"), ClientSession: TOSession,
+					RequestBody:    map[string]interface{}{},
+					RequestHeaders: http.Header{rfc.IfMatch: {rfc.ETag(currentTime)}},
+					Expectations:   utils.CkRequest(utils.HasError(), utils.HasStatus(http.StatusPreconditionFailed)),
+				},
+			},
+			"POST": {
+				"OK when VALID request": {
+					ClientSession: TOSession, RequestBody: map[string]interface{}{
+						"changeType": "create",
+						"deliveryService": generateDeliveryService(t, map[string]interface{}{
+							"ccrDnsTtl":          30,
+							"deepCachingType":    "NEVER",
+							"initialDispersion":  3,
+							"ipv6RoutingEnabled": true,
+							"longDesc":           "long desc",
+							"orgServerFqdn":      "http://example.test",
+							"profileName":        nil,
+							"tenant":             "root",
+							"xmlId":              "test-ds2",
+						}),
+						"status": "draft",
+					},
+					Expectations: utils.CkRequest(utils.NoError(), utils.HasStatus(http.StatusOK)),
+				},
+				"BAD REQUEST when MISSING REQUIRED FIELDS": {
+					ClientSession: TOSession, RequestBody: map[string]interface{}{
+						"changeType": "create",
+						"deliveryService": map[string]interface{}{
+							"type":  "HTTP",
+							"xmlId": "test-ds-fields",
+						},
+						"status": "draft",
+					},
+					Expectations: utils.CkRequest(utils.HasError(), utils.HasStatus(http.StatusBadRequest)),
+				},
+				"BAD REQUEST when VALIDATION RULES ARE NOT FOLLOWED": {
+					ClientSession: TOSession, RequestBody: map[string]interface{}{
+						"changeType": "create",
+						"deliveryService": map[string]interface{}{
+							"ccrDnsTtl":            30,
+							"deepCachingType":      "NEVER",
+							"displayName":          strings.Repeat("X", 49),
+							"dscp":                 0,
+							"geoLimit":             0,
+							"geoProvider":          1,
+							"infoUrl":              "xxx",
+							"initialDispersion":    1,
+							"ipv6RoutingEnabled":   true,
+							"logsEnabled":          true,
+							"longDesc":             "long desc",
+							"missLat":              0.0,
+							"missLong":             0.0,
+							"multiSiteOrigin":      false,
+							"orgServerFqdn":        "http://example.test",
+							"protocol":             0,
+							"qstringIgnore":        0,
+							"rangeRequestHandling": 0,
+							"regionalGeoBlocking":  true,
+							"routingName":          strings.Repeat("X", 1) + "." + strings.Repeat("X", 48),
+							"tenant":               "tenant1",
+							"type":                 "HTTP",
+							"xmlId":                "X " + strings.Repeat("X", 46),
+						},
+						"status": "draft",
+					},
+					Expectations: utils.CkRequest(utils.HasError(), utils.HasStatus(http.StatusBadRequest)),
+				},
+				"BAD REQUEST when NON-DRAFT": {
+					ClientSession: TOSession, RequestBody: map[string]interface{}{
+						"changeType": "create",
+						"deliveryService": map[string]interface{}{
+							"active":               false,
+							"cdnName":              "cdn1",
+							"displayName":          "Testing transitions",
+							"dscp":                 3,
+							"geoLimit":             1,
+							"geoProvider":          1,
+							"initialDispersion":    1,
+							"ipv6RoutingEnabled":   true,
+							"logsEnabled":          true,
+							"missLat":              0.0,
+							"missLong":             0.0,
+							"multiSiteOrigin":      false,
+							"orgServerFqdn":        "http://example.test",
+							"protocol":             0,
+							"qstringIgnore":        0,
+							"rangeRequestHandling": 0,
+							"regionalGeoBlocking":  true,
+							"routingName":          "goodroute",
+							"tenant":               "tenant1",
+							"type":                 "HTTP",
+							"xmlId":                "test-transitions",
+						},
+						"status": "pending",
+					},
+					Expectations: utils.CkRequest(utils.HasError(), utils.HasStatus(http.StatusBadRequest)),
+				},
+				"BAD REQUEST when ALREADY EXISTS": {
+					ClientSession: TOSession, RequestBody: map[string]interface{}{
+						"changeType": "create",
+						"deliveryService": map[string]interface{}{
+							"active":               true,
+							"cdnName":              "cdn1",
+							"displayName":          "Good Kabletown CDN",
+							"dscp":                 1,
+							"geoLimit":             1,
+							"geoProvider":          1,
+							"initialDispersion":    1,
+							"ipv6RoutingEnabled":   true,
+							"logsEnabled":          true,
+							"missLat":              0.0,
+							"missLong":             0.0,
+							"multiSiteOrigin":      false,
+							"orgServerFqdn":        "http://example.test",
+							"protocol":             0,
+							"qstringIgnore":        0,
+							"rangeRequestHandling": 0,
+							"regionalGeoBlocking":  true,
+							"routingName":          "goodroute",
+							"tenant":               "tenant1",
+							"type":                 "HTTP",
+							"xmlId":                "test-ds1",
+						},
+						"status": "draft",
+					},
+					Expectations: utils.CkRequest(utils.HasError(), utils.HasStatus(http.StatusBadRequest)),
+				},
+			},
+			"DELETE": {
+				"OK when VALID request": {
+					EndpointId: GetDeliveryServiceRequestId(t, "test-deletion"), ClientSession: TOSession,
+					Expectations: utils.CkRequest(utils.NoError(), utils.HasStatus(http.StatusOK)),
+				},
+			},
+			"GET AFTER CHANGES": {
+				"OK when CHANGES made": {
+					ClientSession:  TOSession,
+					RequestHeaders: http.Header{rfc.IfModifiedSince: {currentTimeRFC}},
+					Expectations:   utils.CkRequest(utils.NoError(), utils.HasStatus(http.StatusOK)),
+				},
+			},
 		}
-	})
-}
 
-func TestDeliveryServiceRequestRules(t *testing.T) {
-	WithObjs(t, []TCObj{CDNs, Types, Parameters, Tenants}, func() {
-		routingName := strings.Repeat("X", 1) + "." + strings.Repeat("X", 48)
-		// Test the xmlId length and form
-		XMLID := "X " + strings.Repeat("X", 46)
-		displayName := strings.Repeat("X", 49)
-
-		dsr := testData.DeliveryServiceRequests[dsrGood]
-		dsr.DeliveryService.DisplayName = displayName
-		dsr.DeliveryService.RoutingName = routingName
-		dsr.DeliveryService.XMLID = XMLID
-
-		_, _, err := TOSession.CreateDeliveryServiceRequest(dsr)
-		if err == nil {
-			t.Error("expected: validation error, actual: nil")
-		}
-	})
-}
-
-func TestDeliveryServiceRequestTypeFields(t *testing.T) {
-	WithObjs(t, []TCObj{CDNs, Types, Tenants, Parameters}, func() {
-		dsr := testData.DeliveryServiceRequests[dsrBadTenant]
-
-		alerts, _, err := TOSession.CreateDeliveryServiceRequest(dsr)
-		if err != nil {
-			t.Errorf("Error occurred %v", err)
-		}
-
-		found := false
-		for _, alert := range alerts.Alerts {
-			if alert.Level == tc.ErrorLevel.String() {
-				t.Errorf("Expected only succuss-level alerts creating a DSR, got error-level alert: %s", alert.Text)
-			} else if alert.Level == tc.SuccessLevel.String() {
-				t.Logf("Got expected alert creating a DSR: %s", alert.Text)
-				found = true
-			}
-		}
-		if !found {
-			t.Errorf("Expected a success-level alert creating a DSR, got none")
+		for method, testCases := range methodTests {
+			t.Run(method, func(t *testing.T) {
+				for name, testCase := range testCases {
+					dsReq := tc.DeliveryServiceRequest{}
+
+					if testCase.RequestBody != nil {
+						dat, err := json.Marshal(testCase.RequestBody)
+						assert.NoError(t, err, "Error occurred when marshalling request body: %v", err)
+						err = json.Unmarshal(dat, &dsReq)
+						assert.NoError(t, err, "Error occurred when unmarshalling request body: %v", err)
+					}
+
+					switch method {
+					case "GET", "GET AFTER CHANGES":
+						t.Run(name, func(t *testing.T) {
+							if name == "OK when VALID XMLID parameter" {
+								resp, reqInf, err := testCase.ClientSession.GetDeliveryServiceRequestByXMLIDWithHdr(testCase.RequestParams["xmlId"][0], testCase.RequestHeaders)
+								for _, check := range testCase.Expectations {
+									check(t, reqInf, resp, tc.Alerts{}, err)
+								}
+							} else {
+								resp, reqInf, err := testCase.ClientSession.GetDeliveryServiceRequestsWithHdr(testCase.RequestHeaders)
+								for _, check := range testCase.Expectations {
+									check(t, reqInf, resp, tc.Alerts{}, err)
+								}
+							}
+						})
+					case "POST":
+						t.Run(name, func(t *testing.T) {
+							alerts, reqInf, err := testCase.ClientSession.CreateDeliveryServiceRequest(dsReq)
+							for _, check := range testCase.Expectations {
+								check(t, reqInf, nil, alerts, err)
+							}
+						})
+					case "PUT":
+						t.Run(name, func(t *testing.T) {
+							alerts, reqInf, err := testCase.ClientSession.UpdateDeliveryServiceRequestByIDWithHdr(testCase.EndpointId(), dsReq, testCase.RequestHeaders)
+							for _, check := range testCase.Expectations {
+								check(t, reqInf, nil, alerts, err)
+							}
+						})
+					case "DELETE":

Review Comment:
   Why is there a case DELETE, when it is already covered in withobjs_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.

To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [trafficcontrol] rimashah25 commented on a diff in pull request #6758: Refactor Delivery Service Requests

Posted by GitBox <gi...@apache.org>.
rimashah25 commented on code in PR #6758:
URL: https://github.com/apache/trafficcontrol/pull/6758#discussion_r856543814


##########
traffic_ops/testing/api/v4/deliveryservice_requests_test.go:
##########
@@ -16,672 +16,345 @@ package v4
 */
 
 import (
+	"encoding/json"
 	"net/http"
+	"net/url"
 	"strconv"
 	"strings"
 	"testing"
 	"time"
 
 	"github.com/apache/trafficcontrol/lib/go-rfc"
-	tc "github.com/apache/trafficcontrol/lib/go-tc"
-	"github.com/apache/trafficcontrol/lib/go-util"
+	"github.com/apache/trafficcontrol/lib/go-tc"
+	"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"
 )
 
-const (
-	dsrGood      = 0
-	dsrBadTenant = 1
-	dsrRequired  = 2
-	dsrDraft     = 3
-)
-
-// this resets the IDs of things attached to a DS, which needs to be done
-// because the WithObjs flow destroys and recreates those object IDs
-// non-deterministically with each test - BUT, the client method permanently
-// alters the DSR structures by adding these referential IDs. Older clients
-// got away with it by not making 'DeliveryService' a pointer, but to add
-// original/requested fields you need to sometimes allow each to be nil, so
-// this is a problem that needs to be solved at some point.
-// A better solution _might_ be to reload all the test fixtures every time
-// to wipe any and all referential modifications made to any test data, but
-// for now that's overkill.
-func resetDS(ds *tc.DeliveryServiceV4) {
-	if ds == nil {
-		return
-	}
-	ds.CDNID = nil
-	ds.ID = nil
-	ds.ProfileID = nil
-	ds.TenantID = nil
-	ds.TypeID = nil
-}
-
 func TestDeliveryServiceRequests(t *testing.T) {
 	WithObjs(t, []TCObj{CDNs, Types, Parameters, Tenants, DeliveryServiceRequests}, func() {
-		GetTestDeliveryServiceRequestsIMS(t)
-		GetTestDeliveryServiceRequests(t)
-		currentTime := time.Now().UTC().Add(-5 * time.Second)
-		time := currentTime.Format(time.RFC1123)
-		var header http.Header
-		header = make(map[string][]string)
-		header.Set(rfc.IfModifiedSince, time)
-		header.Set(rfc.IfUnmodifiedSince, time)
-		UpdateTestDeliveryServiceRequestsWithLongDescFields(t)
-		UpdateTestDeliveryServiceRequests(t)
-		UpdateTestDeliveryServiceRequestsWithHeaders(t, header)
-		GetTestDeliveryServiceRequestsIMSAfterChange(t, header)
-		header = make(map[string][]string)
-		etag := rfc.ETag(currentTime)
-		header.Set(rfc.IfMatch, etag)
-		UpdateTestDeliveryServiceRequestsWithHeaders(t, header)
-	})
-}
-
-func UpdateTestDeliveryServiceRequestsWithLongDescFields(t *testing.T) {
-	if len(testData.DeliveryServiceRequests) < dsrGood+1 {
-		t.Fatalf("Need at least %d Delivery Service Requests to test updating them", dsrGood+1)
-	}
-
-	// Retrieve the DeliveryServiceRequest by name so we can get the id for the Update
-	dsr := testData.DeliveryServiceRequests[dsrGood]
-	var ds *tc.DeliveryServiceV4
-	if dsr.ChangeType == tc.DSRChangeTypeDelete {
-		dsr.Original.LongDesc1 = util.StrPtr("long desc 1")
-		dsr.Original.LongDesc2 = util.StrPtr("long desc 2")
-		ds = dsr.Original
-	} else {
-		dsr.Requested.LongDesc1 = util.StrPtr("long desc 1")
-		dsr.Requested.LongDesc2 = util.StrPtr("long desc 2")
-		ds = dsr.Requested
-	}
-
-	if ds == nil || ds.XMLID == nil {
-		t.Fatalf("the %dth DSR in the test data had no DeliveryService - or that DeliveryService had no XMLID", dsrGood)
-	}
-
-	opts := client.NewRequestOptions()
-	opts.QueryParameters.Set("xmlId", *ds.XMLID)
-	resp, _, err := TOSession.GetDeliveryServiceRequests(opts)
-	if err != nil {
-		t.Errorf("cannot get Delivery Service Request with XMLID '%s': %v - alerts: %+v", *ds.XMLID, err, resp.Alerts)
-	}
-	if len(resp.Response) == 0 {
-		t.Fatalf("Expected at least one Deliver Service Request to exist with XMLID '%s', but none were found in Traffic Ops", *ds.XMLID)
-	}
-	respDSR := resp.Response[0]
-	if respDSR.ID == nil {
-		t.Fatalf("got a DSR by XMLID '%s' with a null or undefined ID", *ds.XMLID)
-	}
-	var respDS *tc.DeliveryServiceV4
-	if dsr.ChangeType == tc.DSRChangeTypeDelete {
-		respDS = dsr.Original
-		respDSR.Original = respDS
-	} else {
-		respDS = dsr.Requested
-		respDSR.Requested = respDS
-	}
-	expDisplayName := "new display name"
-	respDS.DisplayName = &expDisplayName
-	id := *respDSR.ID
-	_, reqInf, err := TOSession.UpdateDeliveryServiceRequest(id, respDSR, client.RequestOptions{})
-	if err == nil {
-		t.Errorf("expected an error stating that Long Desc 1 and Long Desc 2 fields are not supported in api version 4.0 onwards, but got nothing")
-	}
-	if reqInf.StatusCode != http.StatusBadRequest {
-		t.Errorf("expected a 400 status code, but got %d", reqInf.StatusCode)
-	}
-}
-
-// Note that this test is susceptible to breaking if the structure of the test
-// data's DSRs is altered at all.
-func UpdateTestDeliveryServiceRequestsWithHeaders(t *testing.T, header http.Header) {
-	if len(testData.DeliveryServiceRequests) < dsrGood+1 {
-		t.Fatalf("Need at least %d Delivery Service Requests to test updating them using headers", dsrGood+1)
-	}
-	// Retrieve the DeliveryServiceRequest by name so we can get the id for the Update
-	dsr := testData.DeliveryServiceRequests[dsrGood]
-	var ds *tc.DeliveryServiceV4
-	if dsr.ChangeType == tc.DSRChangeTypeDelete {
-		ds = dsr.Original
-	} else {
-		ds = dsr.Requested
-	}
-	resetDS(ds)
-	if ds == nil || ds.XMLID == nil {
-		t.Fatalf("the %dth DSR in the test data had no Delivery Service, or that Delivery Service had a null or undefined XMLID", dsrGood)
-	}
-	opts := client.NewRequestOptions()
-	opts.Header = header
-	opts.QueryParameters.Set("xmlId", *ds.XMLID)
-	resp, _, err := TOSession.GetDeliveryServiceRequests(opts)
-	if err != nil {
-		t.Errorf("cannot get Delivery Service Request by XMLID '%s': %v - alerts: %+v", *ds.XMLID, err, resp.Alerts)
-	}
-	if len(resp.Response) == 0 {
-		t.Fatal("Length of GET DeliveryServiceRequest is 0")
-	}
-	respDSR := resp.Response[0]
-	if respDSR.ID == nil {
-		t.Fatalf("Got a DSR for XML ID '%s' that had a nil ID", *ds.XMLID)
-	}
-	if respDSR.ChangeType != dsr.ChangeType {
-		t.Fatalf("remote representation of DSR with XMLID '%s' differed from stored data", *ds.XMLID)
-	}
-	var respDS *tc.DeliveryServiceV4
-	if respDSR.ChangeType == tc.DSRChangeTypeDelete {
-		respDS = respDSR.Original
-	} else {
-		respDS = respDSR.Requested
-	}
-
-	respDS.DisplayName = new(string)
-	*respDS.DisplayName = "new display name"
-	opts.QueryParameters.Del("xmlId")
-	_, reqInf, err := TOSession.UpdateDeliveryServiceRequest(*respDSR.ID, respDSR, opts)
-	if err == nil {
-		t.Errorf("Expected error about precondition failed, but got none")
-	}
-	if reqInf.StatusCode != http.StatusPreconditionFailed {
-		t.Errorf("Expected status code 412, got %v", reqInf.StatusCode)
-	}
-}
-
-// Note that this test is susceptible to breaking if the structure of the test
-// data's DSRs is altered at all.
-func GetTestDeliveryServiceRequestsIMSAfterChange(t *testing.T, header http.Header) {
-	if len(testData.DeliveryServiceRequests) < dsrGood+1 {
-		t.Fatalf("Need at least %d Delivery Service Requests to test updating them with IMS", dsrGood+1)
-	}
-	dsr := testData.DeliveryServiceRequests[dsrGood]
-	var ds *tc.DeliveryServiceV4
-	if dsr.ChangeType == tc.DSRChangeTypeDelete {
-		ds = dsr.Original
-	} else {
-		ds = dsr.Requested
-	}
-
-	resetDS(ds)
-	if ds == nil || ds.XMLID == nil {
-		t.Fatalf("the %dth DSR in the test data had no Delivery Service, or that Delivery Service had a null or undefined XMLID", dsrGood)
-	}
-
-	opts := client.NewRequestOptions()
-	opts.Header = header
-	opts.QueryParameters.Set("xmlId", *ds.XMLID)
-	resp, reqInf, err := TOSession.GetDeliveryServiceRequests(opts)
-	if err != nil {
-		t.Fatalf("Expected no error, but got: %v - alerts: %+v", err, resp.Alerts)
-	}
-	if reqInf.StatusCode != http.StatusOK {
-		t.Fatalf("Expected 200 status code, got %v", reqInf.StatusCode)
-	}
-	currentTime := time.Now().UTC()
-	currentTime = currentTime.Add(1 * time.Second)
-	timeStr := currentTime.Format(time.RFC1123)
-	opts.Header.Set(rfc.IfModifiedSince, timeStr)
-	resp, reqInf, err = TOSession.GetDeliveryServiceRequests(opts)
-	if err != nil {
-		t.Fatalf("Expected no error, but got: %v - alerts: %+v", err, resp.Alerts)
-	}
-	if reqInf.StatusCode != http.StatusNotModified {
-		t.Fatalf("Expected 304 status code, got %v", reqInf.StatusCode)
-	}
-}
-
-// Note that this test is susceptible to breaking if the structure of the test
-// data's DSRs is altered at all.
-func CreateTestDeliveryServiceRequests(t *testing.T) {
-	if len(testData.DeliveryServiceRequests) < dsrGood+1 {
-		t.Fatalf("Need at least %d Delivery Service Requests to test creating Delivery Service Requests", dsrGood+1)
-	}
-	dsr := testData.DeliveryServiceRequests[dsrGood]
-	resetDS(dsr.Original)
-	resetDS(dsr.Requested)
-	respDSR, _, err := TOSession.CreateDeliveryServiceRequest(dsr, client.RequestOptions{})
-	if err != nil {
-		t.Errorf("could not create Delivery Service Requests: %v - alerts: %+v", err, respDSR.Alerts)
-	}
 
-}
-
-// Note that this test is susceptible to breaking if the structure of the test
-// data's DSRs is altered at all.
-func TestDeliveryServiceRequestRequired(t *testing.T) {
-	WithObjs(t, []TCObj{CDNs, Types, Parameters, Tenants}, func() {
-		if len(testData.DeliveryServiceRequests) < dsrRequired+1 {
-			t.Fatalf("Need at least %d Delivery Service Requests to test creating a Delivery Service Request missing required fields", dsrRequired+1)
-		}
-		dsr := testData.DeliveryServiceRequests[dsrRequired]
-		resetDS(dsr.Original)
-		resetDS(dsr.Requested)
-		_, _, err := TOSession.CreateDeliveryServiceRequest(dsr, client.RequestOptions{})
-		if err == nil {
-			t.Error("expected: validation error, actual: nil")
+		currentTime := time.Now().UTC().Add(-15 * time.Second)
+		currentTimeRFC := currentTime.Format(time.RFC1123)
+		tomorrow := currentTime.AddDate(0, 0, 1).Format(time.RFC1123)
+
+		methodTests := utils.V4TestCase{
+			"GET": {
+				"NOT MODIFIED when NO CHANGES made": {
+					ClientSession: TOSession, RequestOpts: client.RequestOptions{Header: http.Header{rfc.IfModifiedSince: {tomorrow}}},
+					Expectations: utils.CkRequest(utils.NoError(), utils.HasStatus(http.StatusNotModified)),
+				},
+				"OK when VALID XMLID parameter": {
+					ClientSession: TOSession, RequestOpts: client.RequestOptions{QueryParameters: url.Values{"xmlId": {"test-ds1"}}},
+					Expectations: utils.CkRequest(utils.NoError(), utils.HasStatus(http.StatusOK), utils.ResponseLengthGreaterOrEqual(1),
+						validateGetDSRequestFields(map[string]interface{}{"XMLID": "test-ds1"})),
+				},
+			},
+			"PUT": {
+				"OK when VALID request": {
+					EndpointId: GetDeliveryServiceRequestId(t, "test-ds1"), ClientSession: TOSession,
+					RequestBody: map[string]interface{}{
+						"changeType": "create",
+						"requested": generateDeliveryService(t, map[string]interface{}{
+							"displayName": "NEW DISPLAY NAME",
+							"tenantId":    GetTenantId(t, "tenant1"),
+							"xmlId":       "test-ds1",
+						}),
+						"status": "draft",
+					},
+					Expectations: utils.CkRequest(utils.NoError(), utils.HasStatus(http.StatusOK)),
+				},
+				"OK when UPDATING STATUS FROM DRAFT TO SUBMITTED": {
+					EndpointId: GetDeliveryServiceRequestId(t, "test-ds1"), ClientSession: TOSession,
+					RequestBody: map[string]interface{}{
+						"changeType": "create",
+						"requested": generateDeliveryService(t, map[string]interface{}{
+							"tenantId": GetTenantId(t, "tenant1"),
+							"xmlId":    "test-ds1",
+						}),
+						"status": "submitted",
+					},
+					Expectations: utils.CkRequest(utils.NoError(), utils.HasStatus(http.StatusOK),
+						validatePutDSRequestFields(map[string]interface{}{"STATUS": tc.RequestStatusSubmitted})),
+				},
+				"BAD REQUEST when using LONG DESCRIPTION 2 and 3 fields": {
+					EndpointId: GetDeliveryServiceRequestId(t, "test-ds1"), ClientSession: TOSession,
+					RequestBody: map[string]interface{}{
+						"changeType": "create",
+						"requested": generateDeliveryService(t, map[string]interface{}{
+							"longDesc1": "long desc 1",
+							"longDesc2": "long desc 2",
+							"tenantId":  GetTenantId(t, "tenant1"),
+							"xmlId":     "test-ds1",
+						}),
+						"status": "draft",
+					},
+					Expectations: utils.CkRequest(utils.HasError(), utils.HasStatus(http.StatusBadRequest)),
+				},
+				"PRECONDITION FAILED when updating with IF-UNMODIFIED-SINCE Header": {
+					EndpointId: GetDeliveryServiceRequestId(t, "test-ds1"), ClientSession: TOSession,
+					RequestOpts:  client.RequestOptions{Header: http.Header{rfc.IfUnmodifiedSince: {currentTimeRFC}}},
+					RequestBody:  map[string]interface{}{},
+					Expectations: utils.CkRequest(utils.HasError(), utils.HasStatus(http.StatusPreconditionFailed)),
+				},
+				"PRECONDITION FAILED when updating with IFMATCH ETAG Header": {
+					EndpointId: GetDeliveryServiceRequestId(t, "test-ds1"), ClientSession: TOSession,
+					RequestBody:  map[string]interface{}{},
+					RequestOpts:  client.RequestOptions{Header: http.Header{rfc.IfMatch: {rfc.ETag(currentTime)}}},
+					Expectations: utils.CkRequest(utils.HasError(), utils.HasStatus(http.StatusPreconditionFailed)),
+				},
+			},
+			"POST": {
+				"CREATED when VALID request": {
+					ClientSession: TOSession, RequestBody: map[string]interface{}{
+						"changeType": "create",
+						"requested": generateDeliveryService(t, map[string]interface{}{
+							"ccrDnsTtl":          30,
+							"deepCachingType":    "NEVER",
+							"initialDispersion":  3,
+							"ipv6RoutingEnabled": true,
+							"longDesc":           "long desc",
+							"orgServerFqdn":      "http://example.test",
+							"profileName":        nil,
+							"tenant":             "root",
+							"xmlId":              "test-ds2",
+						}),
+						"status": "draft",
+					},
+					Expectations: utils.CkRequest(utils.NoError(), utils.HasStatus(http.StatusCreated)),
+				},
+				"BAD REQUEST when MISSING REQUIRED FIELDS": {
+					ClientSession: TOSession, RequestBody: map[string]interface{}{
+						"changeType": "create",
+						"requested": map[string]interface{}{
+							"type":  "HTTP",
+							"xmlId": "test-ds-fields",
+						},
+						"status": "draft",
+					},
+					Expectations: utils.CkRequest(utils.HasError(), utils.HasStatus(http.StatusBadRequest)),
+				},
+				"BAD REQUEST when VALIDATION RULES ARE NOT FOLLOWED": {
+					ClientSession: TOSession, RequestBody: map[string]interface{}{
+						"changeType": "create",
+						"requested": map[string]interface{}{
+							"ccrDnsTtl":            30,
+							"deepCachingType":      "NEVER",
+							"displayName":          strings.Repeat("X", 49),
+							"dscp":                 0,
+							"geoLimit":             0,
+							"geoProvider":          1,
+							"infoUrl":              "xxx",
+							"initialDispersion":    1,
+							"ipv6RoutingEnabled":   true,
+							"logsEnabled":          true,
+							"longDesc":             "long desc",
+							"missLat":              0.0,
+							"missLong":             0.0,
+							"multiSiteOrigin":      false,
+							"orgServerFqdn":        "http://example.test",
+							"protocol":             0,
+							"qstringIgnore":        0,
+							"rangeRequestHandling": 0,
+							"regionalGeoBlocking":  true,
+							"routingName":          strings.Repeat("X", 1) + "." + strings.Repeat("X", 48),
+							"tenant":               "tenant1",
+							"type":                 "HTTP",
+							"xmlId":                "X " + strings.Repeat("X", 46),
+						},
+						"status": "draft",
+					},
+					Expectations: utils.CkRequest(utils.HasError(), utils.HasStatus(http.StatusBadRequest)),
+				},
+				"BAD REQUEST when NON-DRAFT": {
+					ClientSession: TOSession, RequestBody: map[string]interface{}{
+						"changeType": "create",
+						"requested": map[string]interface{}{
+							"active":               false,
+							"cdnName":              "cdn1",
+							"displayName":          "Testing transitions",
+							"dscp":                 3,
+							"geoLimit":             1,
+							"geoProvider":          1,
+							"initialDispersion":    1,
+							"ipv6RoutingEnabled":   true,
+							"logsEnabled":          true,
+							"missLat":              0.0,
+							"missLong":             0.0,
+							"multiSiteOrigin":      false,
+							"orgServerFqdn":        "http://example.test",
+							"protocol":             0,
+							"qstringIgnore":        0,
+							"rangeRequestHandling": 0,
+							"regionalGeoBlocking":  true,
+							"routingName":          "goodroute",
+							"tenant":               "tenant1",
+							"type":                 "HTTP",
+							"xmlId":                "test-transitions",
+						},
+						"status": "pending",
+					},
+					Expectations: utils.CkRequest(utils.HasError(), utils.HasStatus(http.StatusBadRequest)),
+				},
+				"BAD REQUEST when ALREADY EXISTS": {
+					ClientSession: TOSession, RequestBody: map[string]interface{}{
+						"changeType": "create",
+						"requested": map[string]interface{}{
+							"active":               true,
+							"cdnName":              "cdn1",
+							"displayName":          "Good Kabletown CDN",
+							"dscp":                 1,
+							"geoLimit":             1,
+							"geoProvider":          1,
+							"initialDispersion":    1,
+							"ipv6RoutingEnabled":   true,
+							"logsEnabled":          true,
+							"missLat":              0.0,
+							"missLong":             0.0,
+							"multiSiteOrigin":      false,
+							"orgServerFqdn":        "http://example.test",
+							"protocol":             0,
+							"qstringIgnore":        0,
+							"rangeRequestHandling": 0,
+							"regionalGeoBlocking":  true,
+							"routingName":          "goodroute",
+							"tenant":               "tenant1",
+							"type":                 "HTTP",
+							"xmlId":                "test-ds1",
+						},
+						"status": "draft",
+					},
+					Expectations: utils.CkRequest(utils.HasError(), utils.HasStatus(http.StatusBadRequest)),
+				},
+			},
+			"DELETE": {

Review Comment:
   Why is there a case DELETE, when it is already covered in withobjs_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.

To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [trafficcontrol] rimashah25 commented on pull request #6758: Refactor Delivery Service Requests

Posted by GitBox <gi...@apache.org>.
rimashah25 commented on PR #6758:
URL: https://github.com/apache/trafficcontrol/pull/6758#issuecomment-1113483638

   All tests cases have been covered.
   LGTM


-- 
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


[GitHub] [trafficcontrol] ericholguin commented on a diff in pull request #6758: Refactor Delivery Service Requests

Posted by GitBox <gi...@apache.org>.
ericholguin commented on code in PR #6758:
URL: https://github.com/apache/trafficcontrol/pull/6758#discussion_r857739163


##########
traffic_ops/testing/api/v3/deliveryservice_requests_test.go:
##########
@@ -16,366 +16,317 @@ package v3
 */
 
 import (
+	"encoding/json"
 	"net/http"
+	"net/url"
 	"strings"
 	"testing"
 	"time"
 
 	"github.com/apache/trafficcontrol/lib/go-rfc"
-	tc "github.com/apache/trafficcontrol/lib/go-tc"
-)
-
-const (
-	dsrGood      = 0
-	dsrBadTenant = 1
-	dsrRequired  = 2
-	dsrDraft     = 3
+	"github.com/apache/trafficcontrol/lib/go-tc"
+	"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 TestDeliveryServiceRequests(t *testing.T) {
 	WithObjs(t, []TCObj{CDNs, Types, Parameters, Tenants, DeliveryServiceRequests}, func() {
-		GetTestDeliveryServiceRequestsIMS(t)
-		GetTestDeliveryServiceRequests(t)
-		currentTime := time.Now().UTC().Add(-5 * time.Second)
-		time := currentTime.Format(time.RFC1123)
-		var header http.Header
-		header = make(map[string][]string)
-		header.Set(rfc.IfModifiedSince, time)
-		header.Set(rfc.IfUnmodifiedSince, time)
-		UpdateTestDeliveryServiceRequests(t)
-		UpdateTestDeliveryServiceRequestsWithHeaders(t, header)
-		GetTestDeliveryServiceRequestsIMSAfterChange(t, header)
-		header = make(map[string][]string)
-		etag := rfc.ETag(currentTime)
-		header.Set(rfc.IfMatch, etag)
-		UpdateTestDeliveryServiceRequestsWithHeaders(t, header)
-	})
-}
-
-func UpdateTestDeliveryServiceRequestsWithHeaders(t *testing.T, header http.Header) {
-	// Retrieve the DeliveryServiceRequest by name so we can get the id for the Update
-	dsr := testData.DeliveryServiceRequests[dsrGood]
-	resp, _, err := TOSession.GetDeliveryServiceRequestByXMLIDWithHdr(dsr.DeliveryService.XMLID, header)
-	if err != nil {
-		t.Errorf("cannot GET DeliveryServiceRequest by name: %v - %v", dsr.DeliveryService.XMLID, err)
-	}
-	if len(resp) == 0 {
-		t.Fatal("Length of GET DeliveryServiceRequest is 0")
-	}
-	respDSR := resp[0]
-	respDSR.DeliveryService.DisplayName = "new display name"
 
-	_, reqInf, err := TOSession.UpdateDeliveryServiceRequestByIDWithHdr(respDSR.ID, respDSR, header)
-	if err == nil {
-		t.Errorf("Expected error about precondition failed, but got none")
-	}
-	if reqInf.StatusCode != http.StatusPreconditionFailed {
-		t.Errorf("Expected status code 412, got %v", reqInf.StatusCode)
-	}
-}
-
-func GetTestDeliveryServiceRequestsIMSAfterChange(t *testing.T, header http.Header) {
-	dsr := testData.DeliveryServiceRequests[dsrGood]
-	_, reqInf, err := TOSession.GetDeliveryServiceRequestByXMLIDWithHdr(dsr.DeliveryService.XMLID, header)
-	if err != nil {
-		t.Fatalf("Expected no error, but got %v", err.Error())
-	}
-	if reqInf.StatusCode != http.StatusOK {
-		t.Fatalf("Expected 200 status code, got %v", reqInf.StatusCode)
-	}
-	currentTime := time.Now().UTC()
-	currentTime = currentTime.Add(1 * time.Second)
-	timeStr := currentTime.Format(time.RFC1123)
-	header.Set(rfc.IfModifiedSince, timeStr)
-	_, reqInf, err = TOSession.GetDeliveryServiceRequestByXMLIDWithHdr(dsr.DeliveryService.XMLID, header)
-	if err != nil {
-		t.Fatalf("Expected no error, but got %v", err.Error())
-	}
-	if reqInf.StatusCode != http.StatusNotModified {
-		t.Fatalf("Expected 304 status code, got %v", reqInf.StatusCode)
-	}
-}
-
-func CreateTestDeliveryServiceRequests(t *testing.T) {
-	t.Log("CreateTestDeliveryServiceRequests")
-
-	dsr := testData.DeliveryServiceRequests[dsrGood]
-	respDSR, _, err := TOSession.CreateDeliveryServiceRequest(dsr)
-	t.Log("Response: ", respDSR)
-	if err != nil {
-		t.Errorf("could not CREATE DeliveryServiceRequests: %v", err)
-	}
-
-}
-
-func TestDeliveryServiceRequestRequired(t *testing.T) {
-	WithObjs(t, []TCObj{CDNs, Types, Parameters, Tenants}, func() {
-		dsr := testData.DeliveryServiceRequests[dsrRequired]
-		_, _, err := TOSession.CreateDeliveryServiceRequest(dsr)
-		if err == nil {
-			t.Error("expected: validation error, actual: nil")
+		currentTime := time.Now().UTC().Add(-15 * time.Second)
+		currentTimeRFC := currentTime.Format(time.RFC1123)
+		tomorrow := currentTime.AddDate(0, 0, 1).Format(time.RFC1123)
+
+		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 XMLID parameter": {
+					ClientSession: TOSession, RequestParams: url.Values{"xmlId": {"test-ds1"}},
+					Expectations: utils.CkRequest(utils.NoError(), utils.HasStatus(http.StatusOK), utils.ResponseLengthGreaterOrEqual(1),
+						validateGetDSRequestFields(map[string]interface{}{"XMLID": "test-ds1"})),
+				},
+			},
+			"PUT": {
+				"OK when VALID request": {
+					EndpointId: GetDeliveryServiceRequestId(t, "test-ds1"), ClientSession: TOSession,
+					RequestBody: map[string]interface{}{
+						"changeType": "create",
+						"deliveryService": generateDeliveryService(t, map[string]interface{}{
+							"displayName": "NEW DISPLAY NAME",
+							"tenantId":    GetTenantId(t, "tenant1"),
+							"xmlId":       "test-ds1",
+						}),
+						"status": "draft",
+					},
+					Expectations: utils.CkRequest(utils.NoError(), utils.HasStatus(http.StatusOK)),
+				},
+				"OK when UPDATING STATUS FROM DRAFT TO SUBMITTED": {
+					EndpointId: GetDeliveryServiceRequestId(t, "test-ds1"), ClientSession: TOSession,
+					RequestBody: map[string]interface{}{
+						"changeType": "create",
+						"deliveryService": generateDeliveryService(t, map[string]interface{}{
+							"tenantId": GetTenantId(t, "tenant1"),
+							"xmlId":    "test-ds1",
+						}),
+						"status": "submitted",
+					},
+					Expectations: utils.CkRequest(utils.NoError(), utils.HasStatus(http.StatusOK)),
+				},
+				"PRECONDITION FAILED when updating with IF-UNMODIFIED-SINCE Header": {
+					EndpointId: GetDeliveryServiceRequestId(t, "test-ds1"), ClientSession: TOSession,
+					RequestHeaders: http.Header{rfc.IfUnmodifiedSince: {currentTimeRFC}},
+					RequestBody:    map[string]interface{}{},
+					Expectations:   utils.CkRequest(utils.HasError(), utils.HasStatus(http.StatusPreconditionFailed)),
+				},
+				"PRECONDITION FAILED when updating with IFMATCH ETAG Header": {
+					EndpointId: GetDeliveryServiceRequestId(t, "test-ds1"), ClientSession: TOSession,
+					RequestBody:    map[string]interface{}{},
+					RequestHeaders: http.Header{rfc.IfMatch: {rfc.ETag(currentTime)}},
+					Expectations:   utils.CkRequest(utils.HasError(), utils.HasStatus(http.StatusPreconditionFailed)),
+				},
+			},
+			"POST": {
+				"OK when VALID request": {
+					ClientSession: TOSession, RequestBody: map[string]interface{}{
+						"changeType": "create",
+						"deliveryService": generateDeliveryService(t, map[string]interface{}{
+							"ccrDnsTtl":          30,
+							"deepCachingType":    "NEVER",
+							"initialDispersion":  3,
+							"ipv6RoutingEnabled": true,
+							"longDesc":           "long desc",
+							"orgServerFqdn":      "http://example.test",
+							"profileName":        nil,
+							"tenant":             "root",
+							"xmlId":              "test-ds2",
+						}),
+						"status": "draft",
+					},
+					Expectations: utils.CkRequest(utils.NoError(), utils.HasStatus(http.StatusOK)),
+				},
+				"BAD REQUEST when MISSING REQUIRED FIELDS": {
+					ClientSession: TOSession, RequestBody: map[string]interface{}{
+						"changeType": "create",
+						"deliveryService": map[string]interface{}{
+							"type":  "HTTP",
+							"xmlId": "test-ds-fields",
+						},
+						"status": "draft",
+					},
+					Expectations: utils.CkRequest(utils.HasError(), utils.HasStatus(http.StatusBadRequest)),
+				},
+				"BAD REQUEST when VALIDATION RULES ARE NOT FOLLOWED": {
+					ClientSession: TOSession, RequestBody: map[string]interface{}{
+						"changeType": "create",
+						"deliveryService": map[string]interface{}{
+							"ccrDnsTtl":            30,
+							"deepCachingType":      "NEVER",
+							"displayName":          strings.Repeat("X", 49),
+							"dscp":                 0,
+							"geoLimit":             0,
+							"geoProvider":          1,
+							"infoUrl":              "xxx",
+							"initialDispersion":    1,
+							"ipv6RoutingEnabled":   true,
+							"logsEnabled":          true,
+							"longDesc":             "long desc",
+							"missLat":              0.0,
+							"missLong":             0.0,
+							"multiSiteOrigin":      false,
+							"orgServerFqdn":        "http://example.test",
+							"protocol":             0,
+							"qstringIgnore":        0,
+							"rangeRequestHandling": 0,
+							"regionalGeoBlocking":  true,
+							"routingName":          strings.Repeat("X", 1) + "." + strings.Repeat("X", 48),
+							"tenant":               "tenant1",
+							"type":                 "HTTP",
+							"xmlId":                "X " + strings.Repeat("X", 46),
+						},
+						"status": "draft",
+					},
+					Expectations: utils.CkRequest(utils.HasError(), utils.HasStatus(http.StatusBadRequest)),
+				},
+				"BAD REQUEST when NON-DRAFT": {
+					ClientSession: TOSession, RequestBody: map[string]interface{}{
+						"changeType": "create",
+						"deliveryService": map[string]interface{}{
+							"active":               false,
+							"cdnName":              "cdn1",
+							"displayName":          "Testing transitions",
+							"dscp":                 3,
+							"geoLimit":             1,
+							"geoProvider":          1,
+							"initialDispersion":    1,
+							"ipv6RoutingEnabled":   true,
+							"logsEnabled":          true,
+							"missLat":              0.0,
+							"missLong":             0.0,
+							"multiSiteOrigin":      false,
+							"orgServerFqdn":        "http://example.test",
+							"protocol":             0,
+							"qstringIgnore":        0,
+							"rangeRequestHandling": 0,
+							"regionalGeoBlocking":  true,
+							"routingName":          "goodroute",
+							"tenant":               "tenant1",
+							"type":                 "HTTP",
+							"xmlId":                "test-transitions",
+						},
+						"status": "pending",
+					},
+					Expectations: utils.CkRequest(utils.HasError(), utils.HasStatus(http.StatusBadRequest)),
+				},
+				"BAD REQUEST when ALREADY EXISTS": {
+					ClientSession: TOSession, RequestBody: map[string]interface{}{
+						"changeType": "create",
+						"deliveryService": map[string]interface{}{
+							"active":               true,
+							"cdnName":              "cdn1",
+							"displayName":          "Good Kabletown CDN",
+							"dscp":                 1,
+							"geoLimit":             1,
+							"geoProvider":          1,
+							"initialDispersion":    1,
+							"ipv6RoutingEnabled":   true,
+							"logsEnabled":          true,
+							"missLat":              0.0,
+							"missLong":             0.0,
+							"multiSiteOrigin":      false,
+							"orgServerFqdn":        "http://example.test",
+							"protocol":             0,
+							"qstringIgnore":        0,
+							"rangeRequestHandling": 0,
+							"regionalGeoBlocking":  true,
+							"routingName":          "goodroute",
+							"tenant":               "tenant1",
+							"type":                 "HTTP",
+							"xmlId":                "test-ds1",
+						},
+						"status": "draft",
+					},
+					Expectations: utils.CkRequest(utils.HasError(), utils.HasStatus(http.StatusBadRequest)),
+				},
+			},
+			"DELETE": {
+				"OK when VALID request": {
+					EndpointId: GetDeliveryServiceRequestId(t, "test-deletion"), ClientSession: TOSession,
+					Expectations: utils.CkRequest(utils.NoError(), utils.HasStatus(http.StatusOK)),
+				},
+			},
+			"GET AFTER CHANGES": {
+				"OK when CHANGES made": {
+					ClientSession:  TOSession,
+					RequestHeaders: http.Header{rfc.IfModifiedSince: {currentTimeRFC}},
+					Expectations:   utils.CkRequest(utils.NoError(), utils.HasStatus(http.StatusOK)),
+				},
+			},
 		}
-	})
-}
 
-func TestDeliveryServiceRequestRules(t *testing.T) {
-	WithObjs(t, []TCObj{CDNs, Types, Parameters, Tenants}, func() {
-		routingName := strings.Repeat("X", 1) + "." + strings.Repeat("X", 48)
-		// Test the xmlId length and form
-		XMLID := "X " + strings.Repeat("X", 46)
-		displayName := strings.Repeat("X", 49)
-
-		dsr := testData.DeliveryServiceRequests[dsrGood]
-		dsr.DeliveryService.DisplayName = displayName
-		dsr.DeliveryService.RoutingName = routingName
-		dsr.DeliveryService.XMLID = XMLID
-
-		_, _, err := TOSession.CreateDeliveryServiceRequest(dsr)
-		if err == nil {
-			t.Error("expected: validation error, actual: nil")
-		}
-	})
-}
-
-func TestDeliveryServiceRequestTypeFields(t *testing.T) {
-	WithObjs(t, []TCObj{CDNs, Types, Tenants, Parameters}, func() {
-		dsr := testData.DeliveryServiceRequests[dsrBadTenant]
-
-		alerts, _, err := TOSession.CreateDeliveryServiceRequest(dsr)
-		if err != nil {
-			t.Errorf("Error occurred %v", err)
-		}
-
-		found := false
-		for _, alert := range alerts.Alerts {
-			if alert.Level == tc.ErrorLevel.String() {
-				t.Errorf("Expected only succuss-level alerts creating a DSR, got error-level alert: %s", alert.Text)
-			} else if alert.Level == tc.SuccessLevel.String() {
-				t.Logf("Got expected alert creating a DSR: %s", alert.Text)
-				found = true
-			}
-		}
-		if !found {
-			t.Errorf("Expected a success-level alert creating a DSR, got none")
+		for method, testCases := range methodTests {
+			t.Run(method, func(t *testing.T) {
+				for name, testCase := range testCases {
+					dsReq := tc.DeliveryServiceRequest{}
+
+					if testCase.RequestBody != nil {
+						dat, err := json.Marshal(testCase.RequestBody)
+						assert.NoError(t, err, "Error occurred when marshalling request body: %v", err)
+						err = json.Unmarshal(dat, &dsReq)
+						assert.NoError(t, err, "Error occurred when unmarshalling request body: %v", err)
+					}
+
+					switch method {
+					case "GET", "GET AFTER CHANGES":
+						t.Run(name, func(t *testing.T) {
+							if name == "OK when VALID XMLID parameter" {
+								resp, reqInf, err := testCase.ClientSession.GetDeliveryServiceRequestByXMLIDWithHdr(testCase.RequestParams["xmlId"][0], testCase.RequestHeaders)
+								for _, check := range testCase.Expectations {
+									check(t, reqInf, resp, tc.Alerts{}, err)
+								}
+							} else {
+								resp, reqInf, err := testCase.ClientSession.GetDeliveryServiceRequestsWithHdr(testCase.RequestHeaders)
+								for _, check := range testCase.Expectations {
+									check(t, reqInf, resp, tc.Alerts{}, err)
+								}
+							}
+						})
+					case "POST":
+						t.Run(name, func(t *testing.T) {
+							alerts, reqInf, err := testCase.ClientSession.CreateDeliveryServiceRequest(dsReq)
+							for _, check := range testCase.Expectations {
+								check(t, reqInf, nil, alerts, err)
+							}
+						})
+					case "PUT":
+						t.Run(name, func(t *testing.T) {
+							alerts, reqInf, err := testCase.ClientSession.UpdateDeliveryServiceRequestByIDWithHdr(testCase.EndpointId(), dsReq, testCase.RequestHeaders)
+							for _, check := range testCase.Expectations {
+								check(t, reqInf, nil, alerts, err)
+							}
+						})
+					case "DELETE":

Review Comment:
   Both the create and delete functions that are used by withobjs are only used for the prerequisites, the only checks those functions perform is that no error occurred, but they do not check status codes or any other validation. And those also do not count as test cases so there is no transparency that post or delete were tested.



##########
traffic_ops/testing/api/v4/deliveryservice_requests_test.go:
##########
@@ -16,672 +16,345 @@ package v4
 */
 
 import (
+	"encoding/json"
 	"net/http"
+	"net/url"
 	"strconv"
 	"strings"
 	"testing"
 	"time"
 
 	"github.com/apache/trafficcontrol/lib/go-rfc"
-	tc "github.com/apache/trafficcontrol/lib/go-tc"
-	"github.com/apache/trafficcontrol/lib/go-util"
+	"github.com/apache/trafficcontrol/lib/go-tc"
+	"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"
 )
 
-const (
-	dsrGood      = 0
-	dsrBadTenant = 1
-	dsrRequired  = 2
-	dsrDraft     = 3
-)
-
-// this resets the IDs of things attached to a DS, which needs to be done
-// because the WithObjs flow destroys and recreates those object IDs
-// non-deterministically with each test - BUT, the client method permanently
-// alters the DSR structures by adding these referential IDs. Older clients
-// got away with it by not making 'DeliveryService' a pointer, but to add
-// original/requested fields you need to sometimes allow each to be nil, so
-// this is a problem that needs to be solved at some point.
-// A better solution _might_ be to reload all the test fixtures every time
-// to wipe any and all referential modifications made to any test data, but
-// for now that's overkill.
-func resetDS(ds *tc.DeliveryServiceV4) {
-	if ds == nil {
-		return
-	}
-	ds.CDNID = nil
-	ds.ID = nil
-	ds.ProfileID = nil
-	ds.TenantID = nil
-	ds.TypeID = nil
-}
-
 func TestDeliveryServiceRequests(t *testing.T) {
 	WithObjs(t, []TCObj{CDNs, Types, Parameters, Tenants, DeliveryServiceRequests}, func() {
-		GetTestDeliveryServiceRequestsIMS(t)
-		GetTestDeliveryServiceRequests(t)
-		currentTime := time.Now().UTC().Add(-5 * time.Second)
-		time := currentTime.Format(time.RFC1123)
-		var header http.Header
-		header = make(map[string][]string)
-		header.Set(rfc.IfModifiedSince, time)
-		header.Set(rfc.IfUnmodifiedSince, time)
-		UpdateTestDeliveryServiceRequestsWithLongDescFields(t)
-		UpdateTestDeliveryServiceRequests(t)
-		UpdateTestDeliveryServiceRequestsWithHeaders(t, header)
-		GetTestDeliveryServiceRequestsIMSAfterChange(t, header)
-		header = make(map[string][]string)
-		etag := rfc.ETag(currentTime)
-		header.Set(rfc.IfMatch, etag)
-		UpdateTestDeliveryServiceRequestsWithHeaders(t, header)
-	})
-}
-
-func UpdateTestDeliveryServiceRequestsWithLongDescFields(t *testing.T) {
-	if len(testData.DeliveryServiceRequests) < dsrGood+1 {
-		t.Fatalf("Need at least %d Delivery Service Requests to test updating them", dsrGood+1)
-	}
-
-	// Retrieve the DeliveryServiceRequest by name so we can get the id for the Update
-	dsr := testData.DeliveryServiceRequests[dsrGood]
-	var ds *tc.DeliveryServiceV4
-	if dsr.ChangeType == tc.DSRChangeTypeDelete {
-		dsr.Original.LongDesc1 = util.StrPtr("long desc 1")
-		dsr.Original.LongDesc2 = util.StrPtr("long desc 2")
-		ds = dsr.Original
-	} else {
-		dsr.Requested.LongDesc1 = util.StrPtr("long desc 1")
-		dsr.Requested.LongDesc2 = util.StrPtr("long desc 2")
-		ds = dsr.Requested
-	}
-
-	if ds == nil || ds.XMLID == nil {
-		t.Fatalf("the %dth DSR in the test data had no DeliveryService - or that DeliveryService had no XMLID", dsrGood)
-	}
-
-	opts := client.NewRequestOptions()
-	opts.QueryParameters.Set("xmlId", *ds.XMLID)
-	resp, _, err := TOSession.GetDeliveryServiceRequests(opts)
-	if err != nil {
-		t.Errorf("cannot get Delivery Service Request with XMLID '%s': %v - alerts: %+v", *ds.XMLID, err, resp.Alerts)
-	}
-	if len(resp.Response) == 0 {
-		t.Fatalf("Expected at least one Deliver Service Request to exist with XMLID '%s', but none were found in Traffic Ops", *ds.XMLID)
-	}
-	respDSR := resp.Response[0]
-	if respDSR.ID == nil {
-		t.Fatalf("got a DSR by XMLID '%s' with a null or undefined ID", *ds.XMLID)
-	}
-	var respDS *tc.DeliveryServiceV4
-	if dsr.ChangeType == tc.DSRChangeTypeDelete {
-		respDS = dsr.Original
-		respDSR.Original = respDS
-	} else {
-		respDS = dsr.Requested
-		respDSR.Requested = respDS
-	}
-	expDisplayName := "new display name"
-	respDS.DisplayName = &expDisplayName
-	id := *respDSR.ID
-	_, reqInf, err := TOSession.UpdateDeliveryServiceRequest(id, respDSR, client.RequestOptions{})
-	if err == nil {
-		t.Errorf("expected an error stating that Long Desc 1 and Long Desc 2 fields are not supported in api version 4.0 onwards, but got nothing")
-	}
-	if reqInf.StatusCode != http.StatusBadRequest {
-		t.Errorf("expected a 400 status code, but got %d", reqInf.StatusCode)
-	}
-}
-
-// Note that this test is susceptible to breaking if the structure of the test
-// data's DSRs is altered at all.
-func UpdateTestDeliveryServiceRequestsWithHeaders(t *testing.T, header http.Header) {
-	if len(testData.DeliveryServiceRequests) < dsrGood+1 {
-		t.Fatalf("Need at least %d Delivery Service Requests to test updating them using headers", dsrGood+1)
-	}
-	// Retrieve the DeliveryServiceRequest by name so we can get the id for the Update
-	dsr := testData.DeliveryServiceRequests[dsrGood]
-	var ds *tc.DeliveryServiceV4
-	if dsr.ChangeType == tc.DSRChangeTypeDelete {
-		ds = dsr.Original
-	} else {
-		ds = dsr.Requested
-	}
-	resetDS(ds)
-	if ds == nil || ds.XMLID == nil {
-		t.Fatalf("the %dth DSR in the test data had no Delivery Service, or that Delivery Service had a null or undefined XMLID", dsrGood)
-	}
-	opts := client.NewRequestOptions()
-	opts.Header = header
-	opts.QueryParameters.Set("xmlId", *ds.XMLID)
-	resp, _, err := TOSession.GetDeliveryServiceRequests(opts)
-	if err != nil {
-		t.Errorf("cannot get Delivery Service Request by XMLID '%s': %v - alerts: %+v", *ds.XMLID, err, resp.Alerts)
-	}
-	if len(resp.Response) == 0 {
-		t.Fatal("Length of GET DeliveryServiceRequest is 0")
-	}
-	respDSR := resp.Response[0]
-	if respDSR.ID == nil {
-		t.Fatalf("Got a DSR for XML ID '%s' that had a nil ID", *ds.XMLID)
-	}
-	if respDSR.ChangeType != dsr.ChangeType {
-		t.Fatalf("remote representation of DSR with XMLID '%s' differed from stored data", *ds.XMLID)
-	}
-	var respDS *tc.DeliveryServiceV4
-	if respDSR.ChangeType == tc.DSRChangeTypeDelete {
-		respDS = respDSR.Original
-	} else {
-		respDS = respDSR.Requested
-	}
-
-	respDS.DisplayName = new(string)
-	*respDS.DisplayName = "new display name"
-	opts.QueryParameters.Del("xmlId")
-	_, reqInf, err := TOSession.UpdateDeliveryServiceRequest(*respDSR.ID, respDSR, opts)
-	if err == nil {
-		t.Errorf("Expected error about precondition failed, but got none")
-	}
-	if reqInf.StatusCode != http.StatusPreconditionFailed {
-		t.Errorf("Expected status code 412, got %v", reqInf.StatusCode)
-	}
-}
-
-// Note that this test is susceptible to breaking if the structure of the test
-// data's DSRs is altered at all.
-func GetTestDeliveryServiceRequestsIMSAfterChange(t *testing.T, header http.Header) {
-	if len(testData.DeliveryServiceRequests) < dsrGood+1 {
-		t.Fatalf("Need at least %d Delivery Service Requests to test updating them with IMS", dsrGood+1)
-	}
-	dsr := testData.DeliveryServiceRequests[dsrGood]
-	var ds *tc.DeliveryServiceV4
-	if dsr.ChangeType == tc.DSRChangeTypeDelete {
-		ds = dsr.Original
-	} else {
-		ds = dsr.Requested
-	}
-
-	resetDS(ds)
-	if ds == nil || ds.XMLID == nil {
-		t.Fatalf("the %dth DSR in the test data had no Delivery Service, or that Delivery Service had a null or undefined XMLID", dsrGood)
-	}
-
-	opts := client.NewRequestOptions()
-	opts.Header = header
-	opts.QueryParameters.Set("xmlId", *ds.XMLID)
-	resp, reqInf, err := TOSession.GetDeliveryServiceRequests(opts)
-	if err != nil {
-		t.Fatalf("Expected no error, but got: %v - alerts: %+v", err, resp.Alerts)
-	}
-	if reqInf.StatusCode != http.StatusOK {
-		t.Fatalf("Expected 200 status code, got %v", reqInf.StatusCode)
-	}
-	currentTime := time.Now().UTC()
-	currentTime = currentTime.Add(1 * time.Second)
-	timeStr := currentTime.Format(time.RFC1123)
-	opts.Header.Set(rfc.IfModifiedSince, timeStr)
-	resp, reqInf, err = TOSession.GetDeliveryServiceRequests(opts)
-	if err != nil {
-		t.Fatalf("Expected no error, but got: %v - alerts: %+v", err, resp.Alerts)
-	}
-	if reqInf.StatusCode != http.StatusNotModified {
-		t.Fatalf("Expected 304 status code, got %v", reqInf.StatusCode)
-	}
-}
-
-// Note that this test is susceptible to breaking if the structure of the test
-// data's DSRs is altered at all.
-func CreateTestDeliveryServiceRequests(t *testing.T) {
-	if len(testData.DeliveryServiceRequests) < dsrGood+1 {
-		t.Fatalf("Need at least %d Delivery Service Requests to test creating Delivery Service Requests", dsrGood+1)
-	}
-	dsr := testData.DeliveryServiceRequests[dsrGood]
-	resetDS(dsr.Original)
-	resetDS(dsr.Requested)
-	respDSR, _, err := TOSession.CreateDeliveryServiceRequest(dsr, client.RequestOptions{})
-	if err != nil {
-		t.Errorf("could not create Delivery Service Requests: %v - alerts: %+v", err, respDSR.Alerts)
-	}
 
-}
-
-// Note that this test is susceptible to breaking if the structure of the test
-// data's DSRs is altered at all.
-func TestDeliveryServiceRequestRequired(t *testing.T) {
-	WithObjs(t, []TCObj{CDNs, Types, Parameters, Tenants}, func() {
-		if len(testData.DeliveryServiceRequests) < dsrRequired+1 {
-			t.Fatalf("Need at least %d Delivery Service Requests to test creating a Delivery Service Request missing required fields", dsrRequired+1)
-		}
-		dsr := testData.DeliveryServiceRequests[dsrRequired]
-		resetDS(dsr.Original)
-		resetDS(dsr.Requested)
-		_, _, err := TOSession.CreateDeliveryServiceRequest(dsr, client.RequestOptions{})
-		if err == nil {
-			t.Error("expected: validation error, actual: nil")
+		currentTime := time.Now().UTC().Add(-15 * time.Second)
+		currentTimeRFC := currentTime.Format(time.RFC1123)
+		tomorrow := currentTime.AddDate(0, 0, 1).Format(time.RFC1123)
+
+		methodTests := utils.V4TestCase{
+			"GET": {
+				"NOT MODIFIED when NO CHANGES made": {
+					ClientSession: TOSession, RequestOpts: client.RequestOptions{Header: http.Header{rfc.IfModifiedSince: {tomorrow}}},
+					Expectations: utils.CkRequest(utils.NoError(), utils.HasStatus(http.StatusNotModified)),
+				},
+				"OK when VALID XMLID parameter": {
+					ClientSession: TOSession, RequestOpts: client.RequestOptions{QueryParameters: url.Values{"xmlId": {"test-ds1"}}},
+					Expectations: utils.CkRequest(utils.NoError(), utils.HasStatus(http.StatusOK), utils.ResponseLengthGreaterOrEqual(1),
+						validateGetDSRequestFields(map[string]interface{}{"XMLID": "test-ds1"})),
+				},
+			},
+			"PUT": {
+				"OK when VALID request": {
+					EndpointId: GetDeliveryServiceRequestId(t, "test-ds1"), ClientSession: TOSession,
+					RequestBody: map[string]interface{}{
+						"changeType": "create",
+						"requested": generateDeliveryService(t, map[string]interface{}{
+							"displayName": "NEW DISPLAY NAME",
+							"tenantId":    GetTenantId(t, "tenant1"),
+							"xmlId":       "test-ds1",
+						}),
+						"status": "draft",
+					},
+					Expectations: utils.CkRequest(utils.NoError(), utils.HasStatus(http.StatusOK)),
+				},
+				"OK when UPDATING STATUS FROM DRAFT TO SUBMITTED": {
+					EndpointId: GetDeliveryServiceRequestId(t, "test-ds1"), ClientSession: TOSession,
+					RequestBody: map[string]interface{}{
+						"changeType": "create",
+						"requested": generateDeliveryService(t, map[string]interface{}{
+							"tenantId": GetTenantId(t, "tenant1"),
+							"xmlId":    "test-ds1",
+						}),
+						"status": "submitted",
+					},
+					Expectations: utils.CkRequest(utils.NoError(), utils.HasStatus(http.StatusOK),
+						validatePutDSRequestFields(map[string]interface{}{"STATUS": tc.RequestStatusSubmitted})),
+				},
+				"BAD REQUEST when using LONG DESCRIPTION 2 and 3 fields": {
+					EndpointId: GetDeliveryServiceRequestId(t, "test-ds1"), ClientSession: TOSession,
+					RequestBody: map[string]interface{}{
+						"changeType": "create",
+						"requested": generateDeliveryService(t, map[string]interface{}{
+							"longDesc1": "long desc 1",
+							"longDesc2": "long desc 2",
+							"tenantId":  GetTenantId(t, "tenant1"),
+							"xmlId":     "test-ds1",
+						}),
+						"status": "draft",
+					},
+					Expectations: utils.CkRequest(utils.HasError(), utils.HasStatus(http.StatusBadRequest)),
+				},
+				"PRECONDITION FAILED when updating with IF-UNMODIFIED-SINCE Header": {
+					EndpointId: GetDeliveryServiceRequestId(t, "test-ds1"), ClientSession: TOSession,
+					RequestOpts:  client.RequestOptions{Header: http.Header{rfc.IfUnmodifiedSince: {currentTimeRFC}}},
+					RequestBody:  map[string]interface{}{},
+					Expectations: utils.CkRequest(utils.HasError(), utils.HasStatus(http.StatusPreconditionFailed)),
+				},
+				"PRECONDITION FAILED when updating with IFMATCH ETAG Header": {
+					EndpointId: GetDeliveryServiceRequestId(t, "test-ds1"), ClientSession: TOSession,
+					RequestBody:  map[string]interface{}{},
+					RequestOpts:  client.RequestOptions{Header: http.Header{rfc.IfMatch: {rfc.ETag(currentTime)}}},
+					Expectations: utils.CkRequest(utils.HasError(), utils.HasStatus(http.StatusPreconditionFailed)),
+				},
+			},
+			"POST": {
+				"CREATED when VALID request": {
+					ClientSession: TOSession, RequestBody: map[string]interface{}{
+						"changeType": "create",
+						"requested": generateDeliveryService(t, map[string]interface{}{
+							"ccrDnsTtl":          30,
+							"deepCachingType":    "NEVER",
+							"initialDispersion":  3,
+							"ipv6RoutingEnabled": true,
+							"longDesc":           "long desc",
+							"orgServerFqdn":      "http://example.test",
+							"profileName":        nil,
+							"tenant":             "root",
+							"xmlId":              "test-ds2",
+						}),
+						"status": "draft",
+					},
+					Expectations: utils.CkRequest(utils.NoError(), utils.HasStatus(http.StatusCreated)),
+				},
+				"BAD REQUEST when MISSING REQUIRED FIELDS": {
+					ClientSession: TOSession, RequestBody: map[string]interface{}{
+						"changeType": "create",
+						"requested": map[string]interface{}{
+							"type":  "HTTP",
+							"xmlId": "test-ds-fields",
+						},
+						"status": "draft",
+					},
+					Expectations: utils.CkRequest(utils.HasError(), utils.HasStatus(http.StatusBadRequest)),
+				},
+				"BAD REQUEST when VALIDATION RULES ARE NOT FOLLOWED": {
+					ClientSession: TOSession, RequestBody: map[string]interface{}{
+						"changeType": "create",
+						"requested": map[string]interface{}{
+							"ccrDnsTtl":            30,
+							"deepCachingType":      "NEVER",
+							"displayName":          strings.Repeat("X", 49),
+							"dscp":                 0,
+							"geoLimit":             0,
+							"geoProvider":          1,
+							"infoUrl":              "xxx",
+							"initialDispersion":    1,
+							"ipv6RoutingEnabled":   true,
+							"logsEnabled":          true,
+							"longDesc":             "long desc",
+							"missLat":              0.0,
+							"missLong":             0.0,
+							"multiSiteOrigin":      false,
+							"orgServerFqdn":        "http://example.test",
+							"protocol":             0,
+							"qstringIgnore":        0,
+							"rangeRequestHandling": 0,
+							"regionalGeoBlocking":  true,
+							"routingName":          strings.Repeat("X", 1) + "." + strings.Repeat("X", 48),
+							"tenant":               "tenant1",
+							"type":                 "HTTP",
+							"xmlId":                "X " + strings.Repeat("X", 46),
+						},
+						"status": "draft",
+					},
+					Expectations: utils.CkRequest(utils.HasError(), utils.HasStatus(http.StatusBadRequest)),
+				},
+				"BAD REQUEST when NON-DRAFT": {
+					ClientSession: TOSession, RequestBody: map[string]interface{}{
+						"changeType": "create",
+						"requested": map[string]interface{}{
+							"active":               false,
+							"cdnName":              "cdn1",
+							"displayName":          "Testing transitions",
+							"dscp":                 3,
+							"geoLimit":             1,
+							"geoProvider":          1,
+							"initialDispersion":    1,
+							"ipv6RoutingEnabled":   true,
+							"logsEnabled":          true,
+							"missLat":              0.0,
+							"missLong":             0.0,
+							"multiSiteOrigin":      false,
+							"orgServerFqdn":        "http://example.test",
+							"protocol":             0,
+							"qstringIgnore":        0,
+							"rangeRequestHandling": 0,
+							"regionalGeoBlocking":  true,
+							"routingName":          "goodroute",
+							"tenant":               "tenant1",
+							"type":                 "HTTP",
+							"xmlId":                "test-transitions",
+						},
+						"status": "pending",
+					},
+					Expectations: utils.CkRequest(utils.HasError(), utils.HasStatus(http.StatusBadRequest)),
+				},
+				"BAD REQUEST when ALREADY EXISTS": {
+					ClientSession: TOSession, RequestBody: map[string]interface{}{
+						"changeType": "create",
+						"requested": map[string]interface{}{
+							"active":               true,
+							"cdnName":              "cdn1",
+							"displayName":          "Good Kabletown CDN",
+							"dscp":                 1,
+							"geoLimit":             1,
+							"geoProvider":          1,
+							"initialDispersion":    1,
+							"ipv6RoutingEnabled":   true,
+							"logsEnabled":          true,
+							"missLat":              0.0,
+							"missLong":             0.0,
+							"multiSiteOrigin":      false,
+							"orgServerFqdn":        "http://example.test",
+							"protocol":             0,
+							"qstringIgnore":        0,
+							"rangeRequestHandling": 0,
+							"regionalGeoBlocking":  true,
+							"routingName":          "goodroute",
+							"tenant":               "tenant1",
+							"type":                 "HTTP",
+							"xmlId":                "test-ds1",
+						},
+						"status": "draft",
+					},
+					Expectations: utils.CkRequest(utils.HasError(), utils.HasStatus(http.StatusBadRequest)),
+				},
+			},
+			"DELETE": {

Review Comment:
   See comment above



-- 
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


[GitHub] [trafficcontrol] rimashah25 commented on a diff in pull request #6758: Refactor Delivery Service Requests

Posted by GitBox <gi...@apache.org>.
rimashah25 commented on code in PR #6758:
URL: https://github.com/apache/trafficcontrol/pull/6758#discussion_r861384058


##########
traffic_ops/testing/api/v4/deliveryservice_requests_test.go:
##########
@@ -16,672 +16,356 @@ package v4
 */
 
 import (
+	"encoding/json"
 	"net/http"
+	"net/url"
 	"strconv"
 	"strings"
 	"testing"
 	"time"
 
 	"github.com/apache/trafficcontrol/lib/go-rfc"
-	tc "github.com/apache/trafficcontrol/lib/go-tc"
-	"github.com/apache/trafficcontrol/lib/go-util"
+	"github.com/apache/trafficcontrol/lib/go-tc"
+	"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"
 )
 
-const (
-	dsrGood      = 0
-	dsrBadTenant = 1
-	dsrRequired  = 2
-	dsrDraft     = 3
-)
-
-// this resets the IDs of things attached to a DS, which needs to be done
-// because the WithObjs flow destroys and recreates those object IDs
-// non-deterministically with each test - BUT, the client method permanently
-// alters the DSR structures by adding these referential IDs. Older clients
-// got away with it by not making 'DeliveryService' a pointer, but to add
-// original/requested fields you need to sometimes allow each to be nil, so
-// this is a problem that needs to be solved at some point.
-// A better solution _might_ be to reload all the test fixtures every time
-// to wipe any and all referential modifications made to any test data, but
-// for now that's overkill.
-func resetDS(ds *tc.DeliveryServiceV4) {
-	if ds == nil {
-		return
-	}
-	ds.CDNID = nil
-	ds.ID = nil
-	ds.ProfileID = nil
-	ds.TenantID = nil
-	ds.TypeID = nil
-}
-
 func TestDeliveryServiceRequests(t *testing.T) {
 	WithObjs(t, []TCObj{CDNs, Types, Parameters, Tenants, DeliveryServiceRequests}, func() {
-		GetTestDeliveryServiceRequestsIMS(t)
-		GetTestDeliveryServiceRequests(t)
-		currentTime := time.Now().UTC().Add(-5 * time.Second)
-		time := currentTime.Format(time.RFC1123)
-		var header http.Header
-		header = make(map[string][]string)
-		header.Set(rfc.IfModifiedSince, time)
-		header.Set(rfc.IfUnmodifiedSince, time)
-		UpdateTestDeliveryServiceRequestsWithLongDescFields(t)
-		UpdateTestDeliveryServiceRequests(t)
-		UpdateTestDeliveryServiceRequestsWithHeaders(t, header)
-		GetTestDeliveryServiceRequestsIMSAfterChange(t, header)
-		header = make(map[string][]string)
-		etag := rfc.ETag(currentTime)
-		header.Set(rfc.IfMatch, etag)
-		UpdateTestDeliveryServiceRequestsWithHeaders(t, header)
-	})
-}
-
-func UpdateTestDeliveryServiceRequestsWithLongDescFields(t *testing.T) {
-	if len(testData.DeliveryServiceRequests) < dsrGood+1 {
-		t.Fatalf("Need at least %d Delivery Service Requests to test updating them", dsrGood+1)
-	}
-
-	// Retrieve the DeliveryServiceRequest by name so we can get the id for the Update
-	dsr := testData.DeliveryServiceRequests[dsrGood]
-	var ds *tc.DeliveryServiceV4
-	if dsr.ChangeType == tc.DSRChangeTypeDelete {
-		dsr.Original.LongDesc1 = util.StrPtr("long desc 1")
-		dsr.Original.LongDesc2 = util.StrPtr("long desc 2")
-		ds = dsr.Original
-	} else {
-		dsr.Requested.LongDesc1 = util.StrPtr("long desc 1")
-		dsr.Requested.LongDesc2 = util.StrPtr("long desc 2")
-		ds = dsr.Requested
-	}
-
-	if ds == nil || ds.XMLID == nil {
-		t.Fatalf("the %dth DSR in the test data had no DeliveryService - or that DeliveryService had no XMLID", dsrGood)
-	}
-
-	opts := client.NewRequestOptions()
-	opts.QueryParameters.Set("xmlId", *ds.XMLID)
-	resp, _, err := TOSession.GetDeliveryServiceRequests(opts)
-	if err != nil {
-		t.Errorf("cannot get Delivery Service Request with XMLID '%s': %v - alerts: %+v", *ds.XMLID, err, resp.Alerts)
-	}
-	if len(resp.Response) == 0 {
-		t.Fatalf("Expected at least one Deliver Service Request to exist with XMLID '%s', but none were found in Traffic Ops", *ds.XMLID)
-	}
-	respDSR := resp.Response[0]
-	if respDSR.ID == nil {
-		t.Fatalf("got a DSR by XMLID '%s' with a null or undefined ID", *ds.XMLID)
-	}
-	var respDS *tc.DeliveryServiceV4
-	if dsr.ChangeType == tc.DSRChangeTypeDelete {
-		respDS = dsr.Original
-		respDSR.Original = respDS
-	} else {
-		respDS = dsr.Requested
-		respDSR.Requested = respDS
-	}
-	expDisplayName := "new display name"
-	respDS.DisplayName = &expDisplayName
-	id := *respDSR.ID
-	_, reqInf, err := TOSession.UpdateDeliveryServiceRequest(id, respDSR, client.RequestOptions{})
-	if err == nil {
-		t.Errorf("expected an error stating that Long Desc 1 and Long Desc 2 fields are not supported in api version 4.0 onwards, but got nothing")
-	}
-	if reqInf.StatusCode != http.StatusBadRequest {
-		t.Errorf("expected a 400 status code, but got %d", reqInf.StatusCode)
-	}
-}
-
-// Note that this test is susceptible to breaking if the structure of the test
-// data's DSRs is altered at all.
-func UpdateTestDeliveryServiceRequestsWithHeaders(t *testing.T, header http.Header) {
-	if len(testData.DeliveryServiceRequests) < dsrGood+1 {
-		t.Fatalf("Need at least %d Delivery Service Requests to test updating them using headers", dsrGood+1)
-	}
-	// Retrieve the DeliveryServiceRequest by name so we can get the id for the Update
-	dsr := testData.DeliveryServiceRequests[dsrGood]
-	var ds *tc.DeliveryServiceV4
-	if dsr.ChangeType == tc.DSRChangeTypeDelete {
-		ds = dsr.Original
-	} else {
-		ds = dsr.Requested
-	}
-	resetDS(ds)
-	if ds == nil || ds.XMLID == nil {
-		t.Fatalf("the %dth DSR in the test data had no Delivery Service, or that Delivery Service had a null or undefined XMLID", dsrGood)
-	}
-	opts := client.NewRequestOptions()
-	opts.Header = header
-	opts.QueryParameters.Set("xmlId", *ds.XMLID)
-	resp, _, err := TOSession.GetDeliveryServiceRequests(opts)
-	if err != nil {
-		t.Errorf("cannot get Delivery Service Request by XMLID '%s': %v - alerts: %+v", *ds.XMLID, err, resp.Alerts)
-	}
-	if len(resp.Response) == 0 {
-		t.Fatal("Length of GET DeliveryServiceRequest is 0")
-	}
-	respDSR := resp.Response[0]
-	if respDSR.ID == nil {
-		t.Fatalf("Got a DSR for XML ID '%s' that had a nil ID", *ds.XMLID)
-	}
-	if respDSR.ChangeType != dsr.ChangeType {
-		t.Fatalf("remote representation of DSR with XMLID '%s' differed from stored data", *ds.XMLID)
-	}
-	var respDS *tc.DeliveryServiceV4
-	if respDSR.ChangeType == tc.DSRChangeTypeDelete {
-		respDS = respDSR.Original
-	} else {
-		respDS = respDSR.Requested
-	}
-
-	respDS.DisplayName = new(string)
-	*respDS.DisplayName = "new display name"
-	opts.QueryParameters.Del("xmlId")
-	_, reqInf, err := TOSession.UpdateDeliveryServiceRequest(*respDSR.ID, respDSR, opts)
-	if err == nil {
-		t.Errorf("Expected error about precondition failed, but got none")
-	}
-	if reqInf.StatusCode != http.StatusPreconditionFailed {
-		t.Errorf("Expected status code 412, got %v", reqInf.StatusCode)
-	}
-}
-
-// Note that this test is susceptible to breaking if the structure of the test
-// data's DSRs is altered at all.
-func GetTestDeliveryServiceRequestsIMSAfterChange(t *testing.T, header http.Header) {
-	if len(testData.DeliveryServiceRequests) < dsrGood+1 {
-		t.Fatalf("Need at least %d Delivery Service Requests to test updating them with IMS", dsrGood+1)
-	}
-	dsr := testData.DeliveryServiceRequests[dsrGood]
-	var ds *tc.DeliveryServiceV4
-	if dsr.ChangeType == tc.DSRChangeTypeDelete {
-		ds = dsr.Original
-	} else {
-		ds = dsr.Requested
-	}
-
-	resetDS(ds)
-	if ds == nil || ds.XMLID == nil {
-		t.Fatalf("the %dth DSR in the test data had no Delivery Service, or that Delivery Service had a null or undefined XMLID", dsrGood)
-	}
-
-	opts := client.NewRequestOptions()
-	opts.Header = header
-	opts.QueryParameters.Set("xmlId", *ds.XMLID)
-	resp, reqInf, err := TOSession.GetDeliveryServiceRequests(opts)
-	if err != nil {
-		t.Fatalf("Expected no error, but got: %v - alerts: %+v", err, resp.Alerts)
-	}
-	if reqInf.StatusCode != http.StatusOK {
-		t.Fatalf("Expected 200 status code, got %v", reqInf.StatusCode)
-	}
-	currentTime := time.Now().UTC()
-	currentTime = currentTime.Add(1 * time.Second)
-	timeStr := currentTime.Format(time.RFC1123)
-	opts.Header.Set(rfc.IfModifiedSince, timeStr)
-	resp, reqInf, err = TOSession.GetDeliveryServiceRequests(opts)
-	if err != nil {
-		t.Fatalf("Expected no error, but got: %v - alerts: %+v", err, resp.Alerts)
-	}
-	if reqInf.StatusCode != http.StatusNotModified {
-		t.Fatalf("Expected 304 status code, got %v", reqInf.StatusCode)
-	}
-}
 
-// Note that this test is susceptible to breaking if the structure of the test
-// data's DSRs is altered at all.
-func CreateTestDeliveryServiceRequests(t *testing.T) {
-	if len(testData.DeliveryServiceRequests) < dsrGood+1 {
-		t.Fatalf("Need at least %d Delivery Service Requests to test creating Delivery Service Requests", dsrGood+1)
-	}
-	dsr := testData.DeliveryServiceRequests[dsrGood]
-	resetDS(dsr.Original)
-	resetDS(dsr.Requested)
-	respDSR, _, err := TOSession.CreateDeliveryServiceRequest(dsr, client.RequestOptions{})
-	if err != nil {
-		t.Errorf("could not create Delivery Service Requests: %v - alerts: %+v", err, respDSR.Alerts)
-	}
-
-}
-
-// Note that this test is susceptible to breaking if the structure of the test
-// data's DSRs is altered at all.
-func TestDeliveryServiceRequestRequired(t *testing.T) {
-	WithObjs(t, []TCObj{CDNs, Types, Parameters, Tenants}, func() {
-		if len(testData.DeliveryServiceRequests) < dsrRequired+1 {
-			t.Fatalf("Need at least %d Delivery Service Requests to test creating a Delivery Service Request missing required fields", dsrRequired+1)
+		currentTime := time.Now().UTC().Add(-15 * time.Second)
+		currentTimeRFC := currentTime.Format(time.RFC1123)
+		tomorrow := currentTime.AddDate(0, 0, 1).Format(time.RFC1123)
+
+		methodTests := utils.V4TestCase{
+			"GET": {
+				"NOT MODIFIED when NO CHANGES made": {
+					ClientSession: TOSession,
+					RequestOpts:   client.RequestOptions{Header: http.Header{rfc.IfModifiedSince: {tomorrow}}},
+					Expectations:  utils.CkRequest(utils.NoError(), utils.HasStatus(http.StatusNotModified)),
+				},
+				"OK when VALID XMLID parameter": {
+					ClientSession: TOSession,
+					RequestOpts:   client.RequestOptions{QueryParameters: url.Values{"xmlId": {"test-ds1"}}},
+					Expectations: utils.CkRequest(utils.NoError(), utils.HasStatus(http.StatusOK), utils.ResponseLengthGreaterOrEqual(1),
+						validateGetDSRequestFields(map[string]interface{}{"XMLID": "test-ds1"})),
+				},
+			},
+			"PUT": {
+				"OK when VALID request": {
+					EndpointId:    GetDeliveryServiceRequestId(t, "test-ds1"),
+					ClientSession: TOSession,
+					RequestBody: map[string]interface{}{
+						"changeType": "create",

Review Comment:
   I am a bit confused. Why is the changeType not `update` but `create` in PUT requests?



-- 
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