-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Run build test pipeline for reduced test scope fore core pipeline #17290
Run build test pipeline for reduced test scope fore core pipeline #17290
Conversation
@@ -3,6 +3,28 @@ const path = require("path"); | |||
const process = require("process"); | |||
const { spawnSync } = require("child_process"); | |||
|
|||
const reducedDependencyTestMatrix = { |
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.
How was this list determined? Is it a manually-selected list of packages we think provide a good balance of coverage vs build time?
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.
Yes I picked them based from the following categories that I think are important/representative. We could fine tune the list if needed.
- Rest core
- Rest level client
- Perf tests
- Non-storage Track 2 hand-written (core-rest-pipeline)
- Management
- Dev tool
- Identity
- Service Bus (core-amqp)
- Storage
- Template (core-http)
- Test utils
- Generated Track 2
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 must say, this list looks like a great balance of everything!
rushCommandFlag = "--only"; | ||
} | ||
|
||
rushRunAll(rushCommandFlag, packageNames); |
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.
Should we rename rushRunAll
to something else since it is no longer running all packages in all cases? perhaps just runRush
?
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.
Sure. Will do in separate cleanup PR if that's fine.
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.
That is fine.
'@azure/identity-vscode', | ||
'@azure/service-bus', | ||
'@azure/storage-blob', | ||
'@azure/template', |
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.
maybe we can drop the template too, it does not buy us anything I think
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 would keep template, it should be very fast and we do want to ensure it doesn't break.
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 do not feel strongly about this but my suggestion is mainly because I can not think of a scenario where template could fail but the others do not.
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.
It took about 20 seconds in js - core - ci pipeline. I picked it because I think it's important in that new packages are often created based on it.
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.
@jeremymeng every package is important for sure but that is beside the point, do we know if there is a scenario where template could break their build or tests where none of the other packages in the list will? I would be surprised if there is one and IMO will indicate template has an unusual design which does not make sense.
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.
Let keep it in the category of "Non-Storage Track 2 core-http based" for now until we migrate everything. Text Analytics covers core-rest-pipeline
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.
Thanks for doing this! I am looking forward to the pipeline has much shorter runtime 🚀
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.
@praveenkuttappan please this @azure-tools/test-recorder
instead of @azure/test-utils-recorder
.
030206e
to
252207d
Compare
changed as per suggestion
/azp run js - identity |
Azure Pipelines successfully started running 1 pipeline(s). |
/azp run js - storage |
Azure Pipelines successfully started running 1 pipeline(s). |
Add a new option to configure predefined set of dependent projects as part of build and test instead of using rush logic to find the dependency. This is required to reduce the test scope in core pipeline from growing 110+ packages to fixed 34 packages which includes core packages too.
Local build time is reduced from 40 mins to 7 mins.