Skip to content

Update config files to prevent duplicated JavaScript tests#2638

Merged
lhw-1 merged 8 commits into
MarkBind:masterfrom
AgentHagu:jest-config-update
Mar 20, 2025
Merged

Update config files to prevent duplicated JavaScript tests#2638
lhw-1 merged 8 commits into
MarkBind:masterfrom
AgentHagu:jest-config-update

Conversation

@AgentHagu

@AgentHagu AgentHagu commented Mar 11, 2025

Copy link
Copy Markdown
Contributor

What is the purpose of this pull request?

  • Documentation update
  • Bug fix
  • Feature addition or enhancement
  • Code maintenance
  • DevOps
  • Improve developer experience
  • Others, please explain:

Overview of changes:
Fixes #2629

As explained in #2629, the core package tests are currently running twice due to having TypeScript and JavaScript versions. To prevent running these tests twice, we can update jest.config.js to only run files that match *.test.ts, thereby ignoring the JavaScript tests.

This PR also updates the tsconfig.json files to further prevent the test.ts files from being compiled to JavaScript in the first place, which also reduces unnecessary output files and keeps the codebase cleaner.

Anything you'd like to highlight/discuss:
As discussed with @gerteck, there shouldn't be a scenario where the TypeScript test passes but the JavaScript version of that test fails -- unless there is an issue during the conversion, which isn't within our scope.

Excluding the test.ts files had an unintended side-effect on ESLint as it previously used tsconfig.json as reference to find TypeScript files to lint. To fix this issue, the PR also adds new tsconfig.lint.json files as the new reference files for .eslintrc.js.

Testing instructions:

  1. Navigate to the core package: cd packages/core
  2. Run the tests: npm run test
  3. Verify that the test report shows 225 total tests, down from the original 445.

Proposed commit message: (wrap lines at 72 characters)
Update config files to prevent duplicated JavaScript tests

The core package tests are currently being run twice because both
TypeScript and JavaScript versions of the test files exist. This leads to
unnecessary duplication of testing, increasing the number of tests
being executed each test run.

To fix this, we update jest.config.js to only run the TypeScript test
files (**/*.test.ts), ignoring the Javascript versions of the tests.

We also update the tsconfig.json files to prevent compilation of
the test.ts files into JavaScript in the first place. This has the added
benefit of reducing the number of unnecessary output files.

A side-effect of excluding the test.ts files in tsconfig.json was that
the ESLint could no longer recognize or find those files for linting. To
fix this, we add tsconfig.lint.json files as the new reference files for
.eslintrc.js


Checklist: ☑️

  • Updated the documentation for feature additions and enhancements
  • Added tests for bug fixes or features
  • Linked all related issues
  • No unrelated changes

Reviewer checklist:

Indicate the SEMVER impact of the PR:

  • Major (when you make incompatible API changes)
  • Minor (when you add functionality in a backward compatible manner)
  • Patch (when you make backward compatible bug fixes)

At the end of the review, please label the PR with the appropriate label: r.Major, r.Minor, r.Patch.

Breaking change release note preparation (if applicable):

  • To be included in the release note for any feature that is made obsolete/breaking

Give a brief explanation note about:

  • what was the old feature that was made obsolete
  • any replacement feature (if any), and
  • how the author should modify his website to migrate from the old feature to the replacement feature (if possible).

@codecov

codecov Bot commented Mar 11, 2025

Copy link
Copy Markdown

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 51.88%. Comparing base (3aa59b5) to head (7a5e84e).

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #2638   +/-   ##
=======================================
  Coverage   51.88%   51.88%           
=======================================
  Files         127      127           
  Lines        5476     5476           
  Branches     1202     1202           
=======================================
  Hits         2841     2841           
  Misses       2340     2340           
  Partials      295      295           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@AgentHagu

AgentHagu commented Mar 11, 2025

Copy link
Copy Markdown
Contributor Author

Currently waiting for #2637 to be merged first before finalizing this PR.

Finalized PR will remove the need for hardcoding '**/filterIconAssets.test.js' into the testMatch

@AgentHagu AgentHagu changed the title Update jest.config.js to ignore duplicated tests Update Jest config to exclude duplicated JavaScript tests Mar 11, 2025
@gerteck

gerteck commented Mar 11, 2025

Copy link
Copy Markdown
Member

Thanks for your effort on this PR! @AgentHagu . This would definitely help immensely with the DevOps process.

One suggestion! - We could also consider excluding the test files from our build pipeline, if it is determined that we don't need the js files at all to run the tests. This can complement the test config, so there are two layers of checks to ensure we don''t run duplicated test files. E.g.:

{
  "extends": "./tsconfig.json",
  "exclude": ["src/**/*.test.ts"]
}

I referenced from stackoverflow

When we add more tests in the future, it can help reduce workload on tsc and also keep our IDE folder less cluttered

@AgentHagu

Copy link
Copy Markdown
Contributor Author

One suggestion! - We could also consider excluding the test files from our build pipeline, if it is determined that we don't need the js files at all to run the tests. This can complement the test config, so there are two layers of checks to ensure we don''t run duplicated test files.

Thanks @gerteck for the catch! I've looked into this suggestion and excluding the *.test.ts files in tsconfig.json will indeed compliment the fix. However, there is a small issue with ESLint since it relies on tsconfig.json and packages/core/tsconfig.json to determine which files to lint. Excluding the test files in tsconfig.json would result in ESLint not being able to find those files, leading to errors like:

0:0  error  Parsing error: ESLint was configured to run on <tsconfigRootDir>/packages\core\test\unit\Page\filterIconAssets.test.ts using parserOptions.project:
- <tsconfigRootDir>/tsconfig.json
- <tsconfigRootDir>/packages\core\tsconfig.json
However, none of those TSConfigs include this file.

To resolve this, I've added new tsconfig,lint.json files for ESLint to refer to, ensuring that ESLint can still properly lint the test files while we exclude them from the build pipeline.

@AgentHagu AgentHagu changed the title Update Jest config to exclude duplicated JavaScript tests Update config files to prevent duplicated JavaScript tests Mar 14, 2025
@AgentHagu AgentHagu marked this pull request as ready for review March 14, 2025 15:06
Since the TypeScript test files are fully adapted
and the tests are passing, we can remove the commented
code from the jest.config.js file.

The remaining JavaScript test will be addressed by
the PR MarkBind#2637.
@AgentHagu AgentHagu force-pushed the jest-config-update branch from 1748119 to f95edb2 Compare March 14, 2025 15:41

@Tim-Siu Tim-Siu left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This Pull Request addresses the problems of duplicated JS/TS tests while also enhancing compilation efficiency.

  1. It configures Jest to only test TypeScript files.
  2. It configures tsconfig so that TypeScript test files are not compiled during the build.
  3. The changes in step 2 break linting. This PR creates separate configuration files for linting, adhering to the practices mentioned in this StackOverflow post.

The PR indeed cuts down the test cases run from 462 (commit ea7cf57) to 231 (commit 87616eb). LGTM!

@lhw-1 lhw-1 added the r.Patch Version resolver: increment by 0.0.1 label Mar 17, 2025
@lhw-1 lhw-1 added this to the v5.6.1 milestone Mar 17, 2025

@lhw-1 lhw-1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for the PR @AgentHagu! This should've been done after the bulk of the TypeScript migration was completed - so at least we've tied up a loose end.

@lhw-1 lhw-1 removed this from the v5.6.1 milestone Mar 20, 2025
@lhw-1 lhw-1 merged commit c8f0e1a into MarkBind:master Mar 20, 2025
@github-actions

Copy link
Copy Markdown

@lhw-1 Each PR must have a SEMVER impact label, please remember to label the PR properly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

r.Patch Version resolver: increment by 0.0.1

Projects

None yet

Development

Successfully merging this pull request may close these issues.

All Markbind Typescript Tests effectively run twice - potentially doubling test time

5 participants