-
Notifications
You must be signed in to change notification settings - Fork 339
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Create API v2 tests #4374
Create API v2 tests #4374
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are a lot of places where a user-agent name is given as to-api-v1-client-tests
(which should say v2 now), and that should probably be a constant somewhere. Not something you need to fix, but a note for the future.
Fixed references to 1.x api and removed the UserDeliveryService tests. Note tests now fail since the 2.x api doesn't exist yet |
That's fine, for the moment. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good, but we should hold off merging until 2.x actually exists, just so that it can add new tests from new endpoints as they are added to 1.5 (looking at you, LetsEncrypt)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The README for the api tests should be updated accordingly to include the branching of v1 vs v2 API Tests.
Also does the integration API test container need to be updated to run 2.x tests on top of the 1.x tests?
How do you run the integration API test container? I can look into changing it if need be. |
I think ideally it would just run the 2.x tests, because the 1.x tests pass and nobody should be modifying the 1.x endpoint handlers |
I think it should have the option to run v1 tests as a regression check and in my opinion by default run both. There is going to be (already is) a lot shared code between v1 and v2 and we need to make sure we are not causing any regressions since we as a community are supporting v1. |
+1 Though, abstracting the tests to support multiple versions sounds like a lot of overhead, especially for the 1.x endpoints that shouldn't be changing much. What if we delete the 1.x tests from the repo, and the operating procedure is for build systems to check out the 4.0.x branch (the latest release before 2.x was added), and run those tests against master TO? And if there are new 1.x tests (which should be rare), we add them to the 4.0.x branch. Seems like that would be easier to manage and maintain. |
I still like the idea of copying the v1 tests (and Go client) to v2 and removing deprecated API usage from the v2 tests (and Go client). Then we can continue to run both v1 and v2 tests, and can cleanly delete all of the v1 tests (and Go client) when the time comes (which should be very soon). The overhead of maintaining two copies of mostly the same tests should be minimal since we'd be deleting everything v1 soon anyways. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like there were some issues rebasing
# github.com/apache/trafficcontrol/traffic_ops/client
traffic_ops/client/deliveryservice.go:134:25: undefined: deliveryServicesByServerEp
traffic_ops/client/deliveryservice.go:367:25: undefined: deliveryServiceStateEp
traffic_ops/client/deliveryservice.go:397:25: undefined: deliveryServiceRoutingEp
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think your merge messed some things up
Tests still failing |
On shamrickus:master, I get 153 API tests run with no failures. Waiting for CDN-in-a-Box to finish loading and running: docker-compose -f docker-compose.yml -f docker-compose.traffic-ops-test.yml up --force-recreate integration |
Really? cuz I'm getting tons of failures:
Are you sure your |
some of these are probably related to the way capabilities are now created. Instead of using "create" and "delete" tests like other objects, they probably ought to be done in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I swear the methods for these configfiles endpoints were/should have been removed from the Go client entirely...
Still getting a failure in the DS tests:
not sure what that means... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tests all pass, removes the bad functionality and typos I introduced (which I suppose is what tests are for)
What does this PR (Pull Request) do?
Which Traffic Control components are affected by this PR?
What is the best way to verify this PR?
Run the go api tests for v2, validate v1 tests still pass
The following criteria are ALL met by this PR
Additional Information