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

Deploy settings in mdapi format #601

Closed
wants to merge 7 commits into from
Closed

Deploy settings in mdapi format #601

wants to merge 7 commits into from

Conversation

maggiben
Copy link
Contributor

@maggiben maggiben commented Jun 21, 2022

Deploy settings in mdapi format

Testing notes:

Link: core in plugin-org main branch is ok, then test the beta commands, besides the regular org, customer has provided a custom org that was failing before you can look it up in this issue: forcedotcom/cli#1526

@W-11163191@

Copy link
Contributor

@mshanemc mshanemc left a comment

Choose a reason for hiding this comment

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

blocking change

  • only on tests
  • tests should assert the object/file are being constructed correctly in the various situations (see comments about testable functions that return objects instead of building an object, converting AND writing to the fs)

src/scratchOrgSettingsGenerator.ts Show resolved Hide resolved
test/unit/scratchOrgSettingsGeneratorTest.ts Outdated Show resolved Hide resolved
@@ -99,7 +86,8 @@ export default class SettingsGenerator {
public async deploySettingsViaFolder(scratchOrg: Org, apiVersion: string): Promise<void> {
const username = scratchOrg.getUsername();
const logger = await Logger.child('deploySettingsViaFolder');
this.createPackageXml(apiVersion);
// this.createPackageXml(apiVersion);
await this.writePackageFile(this.allRecordTypes, this.allbusinessProcesses, this.packageFilePath, apiVersion);
Copy link
Contributor

Choose a reason for hiding this comment

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

since writePackageFile is a method on the class, you could use the this. instance props instead of passing them in.

It might be cleaner to put those in an exported function (could be a different file) that would take inputs and return an object.

Then, your tests could pass a few permutations (with/without record types, BP, objectSettings, etc) and verify that the js objects for CustomObject and package.xml comes back as expected.

That'd be a really clean input/output function test that would cover most of the complexity since the test changes lost all the assertions about the xml contents

src/scratchOrgSettingsGenerator.ts Outdated Show resolved Hide resolved
return output;
output = { ...output, ...{ version: apiVersion } };
const xml = js2xmlparser.parse('Package', output);
this.writer.addToZip(xml, packageFilePath);
Copy link
Contributor

Choose a reason for hiding this comment

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

again, imagine how nice the tests for writePackageFile could be if it were a pure function that took the inputs it needed, didn't use this and returned an object

and let something else convert/write the xml (keeping stuff w/ side effects super simple)

);
expect(addToZipStub.callCount).to.equal(1);
expect(addToZipStub.firstCall.firstArg).to.be.a('string').and.length.to.be.greaterThan(0);
expect(addToZipStub.firstCall.args[1]).to.include(path.join('objects', 'MyObject.object'));
Copy link
Contributor

Choose a reason for hiding this comment

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

it's writing a file, but not verifying that the file is good.

If you tried to do all these permutations in NUTs, that would be a lot of scratch orgs and you'd have to find a way to check the object settings/record types etc.

that's why the "pure function" approach is good for stuff like this where there's a lot of input/output permutations.

@maggiben maggiben requested a review from mshanemc June 28, 2022 14:22
@maggiben
Copy link
Contributor Author

This PR has been closed as a consequence of needing to move this code to V3 here please make your comments on the new PR.
Thank you.

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.

3 participants