-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
Update cmdline and bash taskes to use latest task-lib #11405
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.
This generally looks pretty safe to merge to me once we are able to do a little validation - there shouldn't be any breaking task-lib changes with a minor bump (which we've historically done a good job enforcing for our libraries - other packages are more hit or miss on that).
As you've mentioned, right now we don't have a fantastic testing story. Things we can do to improve that:
- Add unit tests to both of these tasks - the challenge here is that its hard to know what to test and we don't really have a great way to unit test - most of the interesting stuff would probably get mocked anyways.
- Add integration testing. Probably not in this repo since that's not really sustainable for 70ish tasks, but maybe in build canary. The issue here is that right now we don't have a great way to automate running non-published tests. You can publish a version in devfabric or substitute the new code in locally on the agent (the agent caches tasks so you can replace the code after the first run), but that's still not great
- Checked in tasks and/or the ability to run tasks from the GitHub graph. This would make integration testing a whole lot easier.
I think we should do (2) nowish, at least spike on (3) in the next 6 or so months, and (1) maybe never (I don't see a whole lot of value add). Thoughts? Other ideas?
@@ -18,6 +18,6 @@ | |||
"homepage": "https://github.com/Microsoft/azure-pipelines-tasks#readme", | |||
"dependencies": { | |||
"uuid": "^3.0.1", | |||
"vsts-task-lib": "2.7.0" | |||
"azure-pipelines-task-lib": "2.7.0" |
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.
Any reason we're binding to 2.7 instead of most recent?
All of the canary tests (both yaml and UI ones) intrinsically run cmdline, script and inline variations as part of all the definitions. e.g. https://github.com/microsoft/azure-pipelines-canary/search?l=YAML&q=script%3A Of course more can be added. We also have an L0 testing story with hundreds of tests in the task repo as well as E2E testing via L2s and canary. But you are correct that cmdline / script task would have little value if exec is mocked. |
Sure, I don't think we have a good way of doing this before code is checked in though - build canary runs after we've already started to roll out. |
This should be a simple enough change but we need to have a good testing story. Pushing the PR to spark some thought and debate, but we should not merge it until we have a testing plan.