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

fix(compiler): add flag to silence compiler warnings during testing #3719

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

Conversation

amylo
Copy link

@amylo amylo commented Oct 16, 2022

Add compilerLogLevel flag to the Stencil testing config to specify which type of compiler console logs will appear during automated tests. By default, the level is set to "error" so only compiler errors will appear and console warnings will be suppressed. If developers wish to see console warnings again, they simply set compilerLogLevel: 'warn' in their Stencil testing config.

Pull request checklist

Please check if your PR fulfills the following requirements:

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been reviewed and added / updated if needed (for bug fixes / features)
  • Build (npm run build) was run locally and any changes were pushed
  • Unit tests (npm test) were run locally and passed
  • E2E Tests (npm run test.karma.prod) were run locally and passed
  • Prettier (npm run prettier) was run locally and passed

Pull request type

Please check the type of change your PR introduces:

  • Bugfix
  • Feature
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • Documentation content changes
  • Other (please describe):

What is the current behavior?

When executing unit tests for components, the Stencil compiler warns that a property "was modified from within the component" when the developer is simply trying to put their component into a specific state in order to test it. These console warnings end up flooding the terminal making it difficult for the developer to work on their component's unit tests.

GitHub Issue Number: #2832

What is the new behavior?

  • The default behaviour will suppress Stencil compiler's console warnings
  • If the developer wishes to see the Stencil compiler's console warnings during testing, they have to set a property in their Stencil.config.ts file, testing.compilerLogLevel to 'warn':
export const config: Config = {
  testing: {
    compilerLogLevel: 'warn'
  }
  ...
};
  • console errors will continue to display

Does this introduce a breaking change?

  • Yes
  • No

Testing

I tested this change by creating a repository with a Stencil component that had a Prop. In the component's unit test, I modify that Prop to another value and verify that my terminal did not display any Stencil compiler warning.

Then I updated the Stencil.config.ts file testing config with compilerLogLevel: 'warn' and ran the unit test again, this time I saw the Stencil compiler warning:

@Prop() \"buttonRole\" on <my-button> is immutable but was modified from within the component.
    More information: https://stenciljs.com/docs/properties#prop-mutability

Other information

@amylo amylo requested a review from a team as a code owner October 16, 2022 01:54
@github-actions
Copy link
Contributor

github-actions bot commented Oct 16, 2022

@stencil/[email protected] ts
tsc --noEmit --project scripts/tsconfig.json && tsx scripts/tech-debt-burndown-report.ts

--strictNullChecks error report

Typechecking with --strictNullChecks resulted in 1084 errors on this branch.

That's the same number of errors on main, so at least we're not creating new ones!

reports and statistics

Our most error-prone files
Path Error Count
src/dev-server/index.ts 37
src/dev-server/server-process.ts 32
src/compiler/prerender/prerender-main.ts 22
src/runtime/vdom/vdom-render.ts 21
src/runtime/client-hydrate.ts 20
src/screenshot/connector-base.ts 19
src/testing/puppeteer/puppeteer-element.ts 19
src/dev-server/request-handler.ts 15
src/runtime/connected-callback.ts 15
src/compiler/prerender/prerender-optimize.ts 14
src/compiler/sys/stencil-sys.ts 14
src/sys/node/node-sys.ts 14
src/compiler/prerender/prerender-queue.ts 13
src/compiler/sys/in-memory-fs.ts 13
src/runtime/set-value.ts 13
src/compiler/output-targets/output-www.ts 12
src/compiler/transformers/test/parse-vdom.spec.ts 12
src/compiler/transformers/transform-utils.ts 12
src/mock-doc/test/attribute.spec.ts 12
src/compiler/build/compiler-ctx.ts 11
Our most common errors
Typescript Error Code Count
TS2322 352
TS2345 333
TS18048 193
TS18047 76
TS2722 27
TS2532 24
TS2531 19
TS2454 14
TS2790 11
TS2352 9
TS2769 8
TS2416 7
TS2538 4
TS2493 3
TS18046 2
TS2684 1
TS2430 1

Unused exports report

There are 15 unused exports on this PR. That's the same number of errors on main, so at least we're not creating new ones!

Unused exports
File Line Identifier
src/runtime/bootstrap-lazy.ts 21 setNonce
src/screenshot/screenshot-fs.ts 18 readScreenshotData
src/testing/testing-utils.ts 198 withSilentWarn
src/utils/index.ts 145 CUSTOM
src/utils/index.ts 245 NODE_TYPES
src/utils/index.ts 269 normalize
src/utils/index.ts 7 escapeRegExpSpecialCharacters
src/compiler/app-core/app-data.ts 25 BUILD
src/compiler/app-core/app-data.ts 116 Env
src/compiler/app-core/app-data.ts 118 NAMESPACE
src/compiler/fs-watch/fs-watch-rebuild.ts 123 updateCacheFromRebuild
src/compiler/types/validate-primary-package-output-target.ts 82 satisfies
src/compiler/types/validate-primary-package-output-target.ts 82 Record
src/testing/puppeteer/puppeteer-declarations.ts 496 WaitForEventOptions
src/compiler/sys/fetch/write-fetch-success.ts 7 writeFetchSuccessSync

Add "compilerLogLevel" flag to the testing config to specify which type
of dev console logs will appear during automated tests. The default level
is set to "error" so only compiler errors will appear and warnings will be
suppressed.
Copy link
Contributor

PR built and packed!

Download the tarball here: https://github.com/ionic-team/stencil/actions/runs/9584393369/artifacts/1617558252

If your browser saves files to ~/Downloads you can install it like so:

unzip -d ~/Downloads ~/Downloads/stencil-core-4.18.3-dev.1718809754.ebcb17a.tgz.zip && npm install ~/Downloads/stencil-core-4.18.3-dev.1718809754.ebcb17a.tgz

Copy link
Member

@christian-bromann christian-bromann left a comment

Choose a reason for hiding this comment

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

Thanks @amylo for raising the PR. I've have some comments to improve type safety and user input correctness.

* - error (default)
* - warn
*/
compilerLogLevel?: string;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
compilerLogLevel?: string;
compilerLogLevel?: 'error' | 'warn';

@@ -2,6 +2,7 @@ import type * as d from '../../declarations';
import { caughtErrors } from './testing-constants';

let customError: d.ErrorHandler | undefined;
const LOG_LEVELS: string[] = ['warn', 'error'];
Copy link
Member

Choose a reason for hiding this comment

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

Instead of string[] can we use the types defined above?

};

export const consoleDevInfo = (..._: any[]) => {
/* noop for testing */
};

export const setErrorHandler = (handler: d.ErrorHandler | undefined) => (customError = handler);

function allowLog(log: string): boolean {
Copy link
Member

Choose a reason for hiding this comment

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

Can we add a validation here to ensure the user input is correct?

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