-
Notifications
You must be signed in to change notification settings - Fork 143
feat: added RunRelevantTests test-level @W-20152151@ #1644
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
Conversation
package.json
Outdated
| "mime": "2.6.0", | ||
| "minimatch": "^9.0.5", | ||
| "proxy-agent": "^6.4.0", | ||
| "semver": "^7.7.3", |
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.
semver is a library I've used in other places (e.g., Code Analyzer) to do semantic version comparison.
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.
We don't need this though. API versions aren't really semver. I've never seen a format other than xx.x, and in practice it's xx.0. When comparing API versions we generally convert to a number. Here's an example: https://github.com/forcedotcom/source-deploy-retrieve/blob/main/src/resolve/pseudoTypes/agentResolver.ts#L117
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.
Fair enough. I'll stick to the existing pattern.
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.
@shetzel , done!
|
|
||
| # error_invalid_test_level | ||
|
|
||
| TestLevel cannot be '%s' unless API version is %s or later |
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 imagine this message needs to go through some kind of review from a tech writer?
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.
@jshackell-sfdc - can you please review this?
| public constructor(options: MetadataApiDeployOptions) { | ||
| super(options); | ||
| options.apiOptions = { ...MetadataApiDeploy.DEFAULT_OPTIONS.apiOptions, ...options.apiOptions }; | ||
| validateOptions(options); |
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.
Ordinarily, I dislike putting errors in a constructor, but doing it here lets us fail faster at the immediate point of invalidity instead of waiting to do it somewhere else.
| apiOptions: { | ||
| testLevel: 'RunRelevantTests', | ||
| }, | ||
| apiVersion: '8.0', // Tricksy case here: 8.0 is alphabetically after "66.0" but semantically after 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.
mocha doesn't have it.each(), so it's harder to write parameterized tests. I chose to have the tricksiest case be the one I wrote. I'm fine manually adding others if we want. (Sidebar: Do we plan to stay on Mocha indefinitely, or is there a plan to migrate this repo to something like vitest or jest?)
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.
No plans to migrate from mocha. If you want you can write a function and reuse it from the tests.
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.
make an array of params, iterate the array (.map/.forEach/etc) and put an it inside there. There's a lot of example of that in this repo.
If you do that inside a describe it's quite nice
| return fileResponses; | ||
| }; | ||
|
|
||
| const validateOptions = (options: MetadataApiDeployOptions): void => { |
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.
Tried to keep this method open-ended so we can expand on it in the future as needed.
Question: is there a reason for the const x = () => {} instead of function x() {} syntax?
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.
Either are fine. Consistency at least within the file (and ideally the library) should be preferred.
What does this PR do?
Adds support for the new
RunRelevantTeststest level, and validation to ensure that this option is only available in API version 66.0 or later.What issues does this PR fix or reference?
@W-20152151@
Functionality Before
The only TestLevel options were
NoTests,RunLocalTests,RunAllTestsInOrgandRunSpecifiedTests.Functionality After
A new
RunRelevantTestsoption is available.