switch [circleci] service to scrape SVG badge, reorganise routes #3413
Conversation
Default URL is now circleci/build/gh/badges/shields/master?token=abc123 There are redirects on the legacy routes to provide backwards compatibility for existing users
|
I quite like allowing
but you're right that across shields we more commonly use the longhand versions. |
Sounds good to me! |
On a side note, it would be great to add this capability to the frontend: where you can paste in a URL from a service and it transforms it into a badge URL. It's a similar idea to #2420. |
services/circleci/circleci.tester.js
Outdated
| ) | ||
| .expectBadge({ | ||
| label: 'build', | ||
| message: 'invalid response data', | ||
| color: 'lightgrey', | ||
| message: isBuildStatus, | ||
| }) |
There was a problem hiding this comment.
We admittedly don't have anything documented around our practices for redirectors yet (I opened #3402 to track creating those docs), but so far we've been testing them by disabling followRedirect and then checking the status and target url, example below from the vso redirector:
t.create('Build: default branch')
.get('/build/totodem/8cf3ec0e-d0c2-4fcd-8206-ad204f254a96/2.svg', {
followRedirect: false,
})
.expectStatus(301)
.expectHeader(
'Location',
'/azure-devops/build/totodem/8cf3ec0e-d0c2-4fcd-8206-ad204f254a96/2.svg'
)The approach you've taken here obviously validates the redirect route as well. This isn't a blocker for this PR by any means, but I just wanted to make note of it. Happy to shift this topic over to #3402 for discussion in the event folks have opinions on whether we have any preference for redirector testers (leave it up to the developer, prefer one or the other, etc.)
There was a problem hiding this comment.
Yeah I can see reasons to go both ways on this.
On the one hand, the thing we're testing is a redirector, so checking it issues a 301 is a unit testing approach to making sure that component does exactly what its supposed to do.
On the other hand, our service tests are really high-level integration tests, so testing the end-to-end behaviour also makes sense in this context.
There probably isn't a single correct answer to that, but it does make sense to pick one approach and document it as a convention. I'd be happy to go either way on it tbh.
There was a problem hiding this comment.
Agreed there are reasons to go both ways here. When I wrote the first of these I picked this way, though to be honest neither way seems perfect.
I'm inclined to do what's easy to maintain.
A sort of middle-ground option is adding a helper that does a bit of both: check the redirect, then follow it, and finally do a basic check that the final result is not an error badge. Part unit test, part integration test, with a very lightweight assertion.
|
Thanks for taking this on, Chris! And thanks Caleb for reviewing! |
Closes #1064
Closes #1792
Closes #1872
Default URL is now
circleci/build/gh/badges/shields/master?token=abc123There are redirects on the legacy routes to provide backwards compatibility for existing users
For review, it is easier to read this diff if you read each commit in isolation