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

build: Replace jest --ci flag with env variable #23590

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

tylerbutler
Copy link
Member

The Jest CLI --ci flag cannot be set in its config file like other settings, but it defaults to isCI from the is-ci package, which in turn uses ci-info, which supports azure. So the flag should already be set to true by default in CI even without checking the env vars ourselves. However, to be capable of running tests in an environment as close to CI as possible, the test scripts that used --ci have been updated to set the TF_Build environment variable, which is what Azure uses.

@Copilot Copilot bot review requested due to automatic review settings January 16, 2025 21:10

Choose a reason for hiding this comment

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

Copilot reviewed 1 out of 6 changed files in this pull request and generated no comments.

Files not reviewed (5)
  • common/lib/common-utils/package.json: Language not supported
  • examples/client-logger/app-insights-logger/package.json: Language not supported
  • package.json: Language not supported
  • packages/tools/devtools/devtools-test-app/package.json: Language not supported
  • packages/tools/devtools/devtools-view/package.json: Language not supported
@github-actions github-actions bot added area: examples Changes that focus on our examples base: main PRs targeted against main branch labels Jan 16, 2025
@github-actions github-actions bot added the dependencies Pull requests that update a dependency file label Jan 16, 2025
Copy link
Member Author

Choose a reason for hiding this comment

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

Whoa these don't seem right! I just added a new dep tha should already be in the lockfile. I'll try on some other machines.

@jason-ha
Copy link
Contributor

I am thinking that rather than messing with an environment variable, we should just use --ci everywhere we call jest.

@tylerbutler
Copy link
Member Author

I am thinking that rather than messing with an environment variable, we should just use --ci everywhere we call jest.

Maybe? That seems unnecessarily verbose given what the setting controls (according to the docs); I'd rather just remove it altogether. The only practical thing we lose if we do that is the ability to run some tests locally exactly as they're run in CI, but again, given what the flag controls in particular, seems like an acceptable loss for less clutter in the scripts.

But a lot of this is biased because I don't see the value in this flag.

Copy link
Contributor

🔗 No broken links found! ✅

Your attention to detail is admirable.

linkcheck output


> [email protected] ci:check-links /home/runner/work/FluidFramework/FluidFramework/docs
> start-server-and-test "npm run serve -- --no-open" 3000 check-links

1: starting server using command "npm run serve -- --no-open"
and when url "[ 'http://127.0.0.1:3000' ]" is responding with HTTP status code 200
running tests using command "npm run check-links"


> [email protected] serve
> docusaurus serve --no-open

[SUCCESS] Serving "build" directory at: http://localhost:3000/

> [email protected] check-links
> linkcheck http://localhost:3000 --skip-file skipped-urls.txt

Crawling...

Stats:
  170508 links
    1603 destination URLs
    1842 URLs ignored
       0 warnings
       0 errors


Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: examples Changes that focus on our examples base: main PRs targeted against main branch dependencies Pull requests that update a dependency file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants