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

Sm/multiple-flag-bug #609

Merged
merged 6 commits into from
May 11, 2023
Merged

Sm/multiple-flag-bug #609

merged 6 commits into from
May 11, 2023

Conversation

mshanemc
Copy link
Contributor

@mshanemc mshanemc commented May 8, 2023

What does this PR do?

usability improvements to prevent forcedotcom/cli#2117.

explanation: the old commands used comma separated lists for test names. There's no validation on the client or server that the named test actually exists. 🤷🏻

People's tests wouldn't run, but it wasn't an exception. This now provides a warning when you put a comma in the --tests flag OR the --coverage-reporters flag because it's probably a mistake.

What issues does this PR fix or reference?

@W-13168361@

Screenshot 2023-05-08 at 11 02 25 AM

mshanemc and others added 2 commits May 8, 2023 11:16
Co-authored-by: Juliet Shackell <[email protected]>
Co-authored-by: Juliet Shackell <[email protected]>
messages/commonFlags.md Outdated Show resolved Hide resolved
@iowillhoit
Copy link
Contributor

QA NOTES

  • 🟢 Replicated error before linking plugin-deploy-retrieve
  • 🟢 Using spaces works before linking plugin-deploy-retrieve
  • 🟢 Help includes --test flag examples
  • 🟡 Running the following command on a fresh Dreamhouse, showed the flag warning twice
    • sf project deploy start --manifest package.xml --test-level RunSpecifiedTests --dry-run --tests "FileUtilitiesTest,GeoCodingServiceTest,TestPropertyController,TestSampleDataController" --ignore-warnings
(node:61928) Warning: The previous version of this command used a comma-separated list for tests. We've changed how you specify multiple tests, so if you continue using your current syntax, your tests will probably not run as you expect.

If a test name contains a space, enclose it in double quotes.
For multiple test names, use one of the following formats:

- Repeat the flag for multiple test names. --tests Test1 --tests Test2 --tests "Test With Space"
- Separate the test names with spaces --tests Test1 Test2 "Test With Space"
(Use `node --trace-warnings ...` to show where the warning was created)
Warning: The previous version of this command used a comma-separated list for tests. We've changed how you specify multiple tests, so if you continue using your current syntax, your tests will probably not run as you expect.

If a test name contains a space, enclose it in double quotes.
For multiple test names, use one of the following formats:

- Repeat the flag for multiple test names. --tests Test1 --tests Test2 --tests "Test With Space"
- Separate the test names with spaces --tests Test1 Test2 "Test With Space"
  • 🔴 Huh, after linking (and using spaces) I am getting this error. Seems unrelated to these changes, but maybe surfacing from recent ConfigAggregator changes
    • Error (1): Unknown config name: org-metadata-rest-deploy.

@iowillhoit
Copy link
Contributor

QA UPDATE

  • 🟡 Ok, the "Unknown config" is because linked plugins don't honor custom config values. Proceeding with bin/dev
  • 🟢 Since warning message when using bin/dev
  • 🟢 With commas, a warning is logged and 0 tests pass
  • 🟢 With spaces, it works as expected and all tests pass
  • 🟢 With multiple '--tests' flags, it works as expected and all tests pass
  • 🟡 Looks like the commaWarningForMultipleFlags for coverageFormattersFlag isn't needed. When you pass options it will enforce the correct format (no commas). However, removing the options front that flag config does show your warning. Example:
Running: bin/dev project deploy start --manifest package.xml --test-level RunSpecifiedTests --dry-run --tests FileUtilitiesTest --tests GeoCodingServiceTest --tests TestPropertyController --tests TestSampleDataController --ignore-warnings --coverage-formatters json,html
Error (1): Expected --coverage-formatters=json,html to be one of: clover, cobertura, html-spa, html, json, json-summary, lcovonly, none, teamcity, text, text-summary
See more help with --help

@iowillhoit
Copy link
Contributor

QA UPDATE

  • 🟢 Formatters still work after update
  • 🟢 Tests still show warning with commas
  • 🟢 Tests still work with spaces
  • 🟢 Tests still work with multiple --tests flags

@iowillhoit iowillhoit merged commit 0a7506f into main May 11, 2023
@iowillhoit iowillhoit deleted the sm/multiple-flag-bug branch May 11, 2023 17:28
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