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

test/static-code: tweak Typescript config again #20690

Merged

Conversation

allisonkarlitskaya
Copy link
Member

Having checkLib: false in tsconfig.conf is a bad idea: it means that if we want to check .d.ts files, we need a separate invocation of the typescript compiler to do that. It also means that .d.ts files won't be checked in your editor by default. Our workaround for this in test/static-code only checks cockpit.d.ts, and we'll soon have other declaration files.

We've been using checkLib: false as a crutch to avoid reporting errors against node_modules/. The relevant TypeScript issue is still unaddressed: microsoft/TypeScript#30511

Modify test/static-code to only report errors if errors were found on something outside of node_modules, but in that case, report all errors. We do this with two invocations of tsc, to keep things simple, but we'll only do the second invocation if we're reporting errors. To keep things speedy, we turn on "incremental" mode (which writes a cache file to speed things up). .gitignore that. It now takes only ~2s to do an incremental check in the "no errors" case.

By default, tsc outputs a very nice format that shows the exact content of errors, but this doesn't work by default in test/static-code because we capture the output before displaying it. Detect if we're running on a TTY (or if FORCE_COLOR=1 as from our workflows) and enable the "pretty" output in that case.

@allisonkarlitskaya allisonkarlitskaya added the no-test For doc/workflow changes, or experiments which don't need a full CI run, label Jul 2, 2024
@allisonkarlitskaya allisonkarlitskaya requested a review from jelly July 2, 2024 13:45
test/static-code Outdated
node_modules/.bin/tsc --checkJs false --skipLibCheck
# Only display tsc output if it contains errors about something other than node_modules/
# https://github.com/microsoft/TypeScript/issues/30511
if tsc --checkJs false --pretty false | grep -Eqv '^(node_modules| )'; then
Copy link
Member

Choose a reason for hiding this comment

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

oh-uh:

./test/static-code: line 74: tsc: command not found

Need to call this with full path, too.

Having `checkLib: false` in `tsconfig.conf` is a bad idea: it means that
if we want to check `.d.ts` files, we need a separate invocation of the
typescript compiler to do that.  It also means that `.d.ts` files won't
be checked in your editor by default.  Our workaround for this in
`test/static-code` only checks `cockpit.d.ts`, and we'll soon have other
declaration files.

We've been using `checkLib: false` as a crutch to avoid reporting errors
against `node_modules/`.  The relevant TypeScript issue is still
unaddressed: microsoft/TypeScript#30511

Modify `test/static-code` to only report errors if errors were found on
something outside of node_modules, but in that case, report all errors.
We do this with two invocations of `tsc`, to keep things simple, but
we'll only do the second invocation if we're reporting errors.  To keep
things speedy, we turn on "incremental" mode (which writes a cache file
to speed things up).  `.gitignore` that.  It now takes only ~2s to do an
incremental check in the "no errors" case.

By default, `tsc` outputs a very nice format that shows the exact
content of errors, but this doesn't work by default in
`test/static-code` because we capture the output before displaying it.
Detect if we're running on a TTY (or if `FORCE_COLOR=1` as from our
workflows) and enable the "pretty" output in that case.
@martinpitt
Copy link
Member

rawhide started to fail:

Failed to resolve the transaction:
No match for argument: reportd

Is ABRT being aborted? I faintly remember that someone said something about that at devconf, but I don't remember any details. This is Yet Another Pilot Thing™

Copy link
Member

@martinpitt martinpitt left a comment

Choose a reason for hiding this comment

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

Thanks!

@allisonkarlitskaya allisonkarlitskaya merged commit ffe6264 into cockpit-project:main Jul 2, 2024
29 of 30 checks passed
@allisonkarlitskaya allisonkarlitskaya deleted the tsc-once branch July 2, 2024 16:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no-test For doc/workflow changes, or experiments which don't need a full CI run,
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants