Skip to content
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

Add matrix-based testing #2323

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Add matrix-based testing #2323

wants to merge 3 commits into from

Conversation

hallipr
Copy link
Member

@hallipr hallipr commented Mar 11, 2025

No description provided.

@heaths heaths changed the title Add matrix based testing Add matrix-based testing Mar 11, 2025
Copy link
Member

@heaths heaths left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apart from the one comment, I don't see anything obviously wrong, but @benbp might be a better reviewer here. And, at least as of when I wrote this, all the pipelines failed so there's something clearly wrong but I couldn't say what from what I see. But I don't see anything to hold the PR up.

@heaths heaths requested a review from benbp March 11, 2025 23:45
Comment on lines +43 to +44
SparseCheckoutPaths:
- /*
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
SparseCheckoutPaths:
- /*

I don't think you need this? It's just for checking out files that may be necessary for matrix generation outside of eng+service directory.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think for PR matrix generation, I'll be back to needing it because I use cargo metadata in LanguageSettings to get package metadata

Copy link
Member Author

@hallipr hallipr Mar 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So ci.yml will need all the code because any of it may need to be built. Depending on how cargo handles partial workspaces, we may still need all the cargo.tomls to do the cargo metadata call even when working with a single service directory.

Comment on lines +13 to +18
default:
- Name: rust_ci_test_base
Path: eng/pipelines/templates/stages/platform-matrix.json
Selection: sparse
NonSparseParameters: RustToolchainName
GenerateVMJobs: true
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The way the matrix is set up now, you could specify it with all instead. Though perhaps you have it with NonSparseParameters to handle when more matrix parameters are added?

Suggested change
default:
- Name: rust_ci_test_base
Path: eng/pipelines/templates/stages/platform-matrix.json
Selection: sparse
NonSparseParameters: RustToolchainName
GenerateVMJobs: true
default:
- Name: rust_ci_test_base
Path: eng/pipelines/templates/stages/platform-matrix.json
Selection: all
GenerateVMJobs: true

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wasn't sure how selection: all would interact with sparse coming from the PR matrix generation

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants