Skip to content

docs(scripts): correct Get-ChangedTestFiles.ps1 usage in tests README - #3230

Open
Mohammed Alkindi (MohammedAlkindi) wants to merge 2 commits into
microsoft:mainfrom
MohammedAlkindi:docs/get-changed-test-files-readme
Open

Mohammed Alkindi (MohammedAlkindi) wants to merge 2 commits into
microsoft:mainfrom
MohammedAlkindi:docs/get-changed-test-files-readme

Conversation

@MohammedAlkindi

Copy link
Copy Markdown
Contributor

Pull Request

Description

The tests README says -BaseBranch defaults to main, but the script has no default, so the no-argument example throws. Marks -BaseBranch required, lists the other defaults and the HAS_CHANGES and TEST_PATHS outputs, and uses -BaseBranch main in the example.

Related Issue(s)

None.

Type of Change

  • Documentation update

Sample Prompts (for AI Artifact Contributions)

N/A

Testing

The old example threw under pwsh 7. markdownlint-cli2, cspell, Format-MarkdownTables.ps1 -Check and Validate-MarkdownFrontmatter.ps1 pass on the changed file.

Checklist

Required Checks

  • Documentation is updated (if applicable)
  • Files follow existing naming conventions
  • Changes are backwards compatible (if applicable)
  • Tests added for new functionality (if applicable)

AI Artifact Contributions

N/A

Required Local Checks

  • Local validation aggregate: npm run validate:local
  • Documentation validation (if docs changed): npm run validate:docs
  • Spell checking: npm run spell-check
  • Link validation: npm run lint:md-links

Security Considerations

  • This PR does not contain any sensitive or NDA information
  • Any new dependencies have been reviewed for security issues
  • Security-related scripts follow the principle of least privilege

Additional Notes

None.

BaseBranch has no default and the script throws without it, so the no-argument example failed. Document the required parameter, the origin/ prefix, the remaining defaults and the HAS_CHANGES and TEST_PATHS outputs.

@bindsi Marcel Bindseil (bindsi) left a comment •

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hi Mohammed Alkindi (@MohammedAlkindi), thank you for this contribution and for all the fixes you've been sending lately. They're much appreciated! Before we continue with the review, could you please take care of a few process items:

1. Complete all required checks

The Required Local Checks in the description are still unchecked. Please run these locally, fix anything they report, and tick each box once it passes (or mark it N/A with a short reason):

  • npm run validate:local
  • npm run validate:docs (if docs changed)
  • npm run spell-check
  • npm run lint:md-links

Please also go through the remaining Required Checks and Security Considerations items so every box is either checked or explicitly marked N/A.

2. Link an issue

This PR doesn't reference an issue yet. Please check whether one already exists for this problem. If not, please create one using the appropriate issue template. Then add it under Related Issue(s) with a closing keyword (for example Fixes #1234) so it is closed automatically on merge.

Thanks again! I'm happy to take another look as soon as these are in place.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thank you for correcting the BaseBranch requirement and expanding the parameter and output reference. I am requesting changes for two documentation contracts that can still mislead callers: the relative root defaults depend on the caller's working directory, and the named outputs are published only in supported CI environments. Please also reconcile the required checklist items and add the exact validation commands and concise results to the PR description.

Comment thread scripts/tests/README.md
directly; changes are diffed against `origin/<BaseBranch>`)
* `-FileFilter` - Glob patterns for filtering changed files (defaults to
`*.ps1`, `*.psm1`)
* `-SkillsRoot` - Root path for skill scripts (defaults to `.github/skills`)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thank you for documenting these defaults. Both relative paths are resolved from the caller's current working directory, so invoking the script from outside the repository root can silently miss matching tests. Please state that the examples must run from the repository root, document the resolution rule, or change the implementation to resolve the defaults from a stable repository path.

Comment thread scripts/tests/README.md Outdated

#### Outputs

When run directly, the script publishes its results through `Set-CIEnv`:

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Set-CIEnv does not create these named values during ordinary local execution; its local branch only emits a verbose message. Please qualify this as supported-CI behavior and describe what local callers can observe, or add a separate local output contract.

Path resolution is relative to the caller's working directory rather
than the script, and Set-CIEnv publishes HAS_CHANGES and TEST_PATHS only
on a direct run in a supported CI environment.
@MohammedAlkindi

Copy link
Copy Markdown
Contributor Author

Both review notes on scripts/tests/README.md are addressed in 81657cb. The README now says the root defaults, the git diff pathspec and the changed-test lookup resolve against the current working directory, so the script runs from the repository root. The Outputs section limits Set-CIEnv publishing to supported CI and says a local run publishes neither value but still lists the matching test files on the console.

This branch has not been deployed

No deployments
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