Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions messages/sdr.md
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,10 @@ Could not find parent type for %s (%s)

Component conversion failed: %s

# error_invalid_test_level

TestLevel cannot be '%s' unless API version is %s or later
Copy link
Contributor Author

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?

Copy link
Contributor

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?


# error_merge_metadata_target_unsupported

Merge convert for metadata target format currently unsupported
Expand Down
2 changes: 2 additions & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
"@salesforce/kit": "^3.2.4",
"@salesforce/ts-types": "^2.0.12",
"@salesforce/types": "^1.5.0",
"@types/semver": "^7.7.1",
"fast-levenshtein": "^3.0.0",
"fast-xml-parser": "^4.5.3",
"got": "^11.8.6",
Expand All @@ -38,6 +39,7 @@
"mime": "2.6.0",
"minimatch": "^9.0.5",
"proxy-agent": "^6.4.0",
"semver": "^7.7.3",
Copy link
Contributor Author

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.

Copy link
Contributor

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

Copy link
Contributor Author

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@shetzel , done!

"yaml": "^2.8.1"
},
"devDependencies": {
Expand Down
13 changes: 13 additions & 0 deletions src/client/metadataApiDeploy.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import { format } from 'node:util';
import { isString } from '@salesforce/ts-types';
import JSZip from 'jszip';
import fs from 'graceful-fs';
import semver from 'semver';
import { Lifecycle } from '@salesforce/core/lifecycle';
import { Messages } from '@salesforce/core/messages';
import { SfError } from '@salesforce/core/sfError';
Expand Down Expand Up @@ -116,6 +117,7 @@ export class MetadataApiDeploy extends MetadataTransfer<
public constructor(options: MetadataApiDeployOptions) {
super(options);
options.apiOptions = { ...MetadataApiDeploy.DEFAULT_OPTIONS.apiOptions, ...options.apiOptions };
validateOptions(options);
Copy link
Contributor Author

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.

this.options = Object.assign({}, options);
this.isRestDeploy = !!options.apiOptions?.rest;
this.registry = options.registry ?? new RegistryAccess();
Expand Down Expand Up @@ -524,6 +526,17 @@ const buildFileResponsesFromComponentSet =
}
return fileResponses;
};

const validateOptions = (options: MetadataApiDeployOptions): void => {
Copy link
Contributor Author

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?

Copy link
Contributor

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.

const runningRelevantTestsOnly = options.apiOptions?.testLevel === 'RunRelevantTests';
const beforeApiV66 = options.apiVersion && semver.lt(semver.coerce(options.apiVersion)!, '66.0.0');
if (runningRelevantTestsOnly && beforeApiV66) {
throw new SfError(
messages.getMessage('error_invalid_test_level', ['RunRelevantTests', '66.0']),
'InvalidTestLevelSelection'
);
}
};
/**
* register a listener to `scopedPreDeploy`
*/
Expand Down
2 changes: 1 addition & 1 deletion src/client/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -364,7 +364,7 @@ export type MetadataApiDeployOptions = {
runAllTests?: boolean;
runTests?: string[];
singlePackage?: boolean;
testLevel?: 'NoTestRun' | 'RunSpecifiedTests' | 'RunLocalTests' | 'RunAllTestsInOrg';
testLevel?: 'NoTestRun' | 'RunSpecifiedTests' | 'RunLocalTests' | 'RunAllTestsInOrg' | 'RunRelevantTests';
/**
* Set to true to use the REST API for deploying.
*/
Expand Down
21 changes: 21 additions & 0 deletions test/client/metadataApiDeploy.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1256,6 +1256,27 @@ describe('MetadataApiDeploy', () => {
expect(mdOpts.zipPath).to.equal('foo/myZip.zip');
});

it('should disallow "RunRelevantTests" for API versions <66.0', () => {
const constructorError = {
name: 'InvalidTestLevelSelection',
message: messages.getMessage('error_invalid_test_level', ['RunRelevantTests', '66.0']),
};
try {
new MetadataApiDeploy({
usernameOrConnection: 'testing',
apiOptions: {
testLevel: 'RunRelevantTests',
},
apiVersion: '8.0', // Tricksy case here: 8.0 is alphabetically after "66.0" but semantically after it.
Copy link
Contributor Author

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?)

Copy link
Contributor

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.

Copy link
Contributor

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

});
assert.fail('Should have thrown an error');
} catch (e) {
assert(e instanceof Error);
expect(e.name).to.equal(constructorError.name);
expect(e.message).to.equal(constructorError.message);
}
});

it('should allow mdapi path', () => {
const mdApiDeploy = new MetadataApiDeploy({
usernameOrConnection: 'testing',
Expand Down
5 changes: 5 additions & 0 deletions yarn.lock
Original file line number Diff line number Diff line change
Expand Up @@ -1039,6 +1039,11 @@
resolved "https://registry.yarnpkg.com/@types/semver/-/semver-7.5.8.tgz#8268a8c57a3e4abd25c165ecd36237db7948a55e"
integrity sha512-I8EUhyrgfLrcTkzV3TSsGyl1tSuPrEDzr0yd5m90UgNxQkyDXULk3b6MlQqTCpZpNtWe1K0hzclnZkTcLBe2UQ==

"@types/semver@^7.7.1":
version "7.7.1"
resolved "https://registry.yarnpkg.com/@types/semver/-/semver-7.7.1.tgz#3ce3af1a5524ef327d2da9e4fd8b6d95c8d70528"
integrity sha512-FmgJfu+MOcQ370SD0ev7EI8TlCAfKYU+B4m5T3yXc1CiRN94g/SZPtsCkk506aUDtlMnFZvasDwHHUcZUEaYuA==

"@types/shelljs@^0.8.15":
version "0.8.15"
resolved "https://registry.yarnpkg.com/@types/shelljs/-/shelljs-0.8.15.tgz#22c6ab9dfe05cec57d8e6cb1a95ea173aee9fcac"
Expand Down
Loading