-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
Support for Test at Scale Badge, run [TAS] #7612
Conversation
|
Thanks for the PR! Did one very quick glance over this and a few initial thoughts:
|
Hey @calebcartwright, Thanks for the suggestions.
Thanks. |
Could you share some more info about the API surface? It gives me pause when I see "badge" as part of the path as it implies it's a specialized endpoint with minimal data for a single badge purpose. That's not necessarily a problem, but as I see it, part of the reason why you can't incorporate our existing functions (which provide end-user features around customization) is because the API response is so minimal. I would think/hope there could be an endpoint that would provide some of the standard details around passed/failed/skipped/etc. that we see with the various other platforms we integrate with. |
@calebcartwright On test-at-scale [TAS] we are not running all test cases for each build. We try to execute minimum number of test cases that are affected by the latest PR changes. Here's the api example. |
Thank you for the reply @nevilm-lt, but I'm not sure you've really responded to the point/topic. We don't want Shields to be locked into an API from a platform that's trying to dictate a specific type of badge down to basic rendering concerns. We want to consume standard APIs that provide data points relevant to general users of the platform, such that we are then able to offer our users a range of options for badges as opposed to assuming/pushing a single use case. We're absolutely on board with being able to provide our users with a TAS badge that displays the test count + duration, but I don't want us to be coupled to an API endpoint that only allows that badge because that really shouldn't be the only one. |
Hi @calebcartwright, We have updated the badge to show standard information as other badges provides. i.e. Passed, Failed, Skipped etc. Also we have used standard utility functions to render the badge. Please feel free to let us know if there are any other changes required. Thanks. |
Any updates on this @calebcartwright ? |
Awesome, thanks for the updates! To clarify a bit on the prior discussion, if you'd like to be able to show a badge that includes the duration in addition to all the other variants that's perfectly alright as well. Things like okay here based on a quick glance, so I can either give this a more thorough review as-is if you're fine with the existing badges, or I can hold off/suggest some next steps if you'd like to get a count+duration badge variant too. |
Hi @calebcartwright, |
services/tas/tas-tests.tester.js
Outdated
|
||
t.create('tas tests').get('/github/tasdemo/axios.json').expectBadge({ | ||
label: 'tests', | ||
message: Joi.string(), |
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.
Can we make this more specific. This isn't really a useful test as an error message will match Joi.string()
too. There are some validators in services/test-validators.js
for testing this type of test summary badge:
shields/services/test-validators.js
Lines 135 to 154 in 6e80336
const isDefaultTestTotals = makeTestTotalsValidator({ | |
passed: 'passed', | |
failed: 'failed', | |
skipped: 'skipped', | |
}) | |
const isDefaultCompactTestTotals = makeCompactTestTotalsValidator({ | |
passed: '✔', | |
failed: '✘', | |
skipped: '➟', | |
}) | |
const isCustomTestTotals = makeTestTotalsValidator({ | |
passed: 'good', | |
failed: 'bad', | |
skipped: 'n/a', | |
}) | |
const isCustomCompactTestTotals = makeCompactTestTotalsValidator({ | |
passed: '💃', | |
failed: '🤦♀️', | |
skipped: '🤷', | |
}) |
Also can we have a couple of tests for the behaviour in the 401 and 404 cases.
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.
added.
Hey @chris48s, We have added the test cases for multiple behaviours which mocks specific use cases. Please let us know if there are anything else we can do. Thanks. |
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.
Overall looks good to me! Believe we're in the home stretch now, couple final minor items
services/tas/tas-tests.service.js
Outdated
namedParams: { | ||
provider: 'github', | ||
org: 'test-at-scale', | ||
repo: 'badge-demo', | ||
}, |
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.
Sorry I missed this before but is this a valid, public project? I'm not seeing it on GitHub and the badge is unsurprisingly showing a not found. We really like to have these examples be valid projects so that users can actually see/work with them in the modal window.
If it's something you all are planning on setting up in the near future that's alright, but otherwise it would probably be better to swap in a real target
https://shields-staging-pr-7612.herokuapp.com/tas/tests/github/test-at-scale/badge-demo?compact_message
Using the same target as the tests would suffice too:
https://shields-staging-pr-7612.herokuapp.com/tas/tests/github/tasdemo/axios
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.
@calebcartwright Thanks for pointing out. I've updated the example project.
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.
Awesome, thanks so much!
Added
Usage
Badge