Skip to content

Conversation

@jupblb
Copy link
Member

@jupblb jupblb commented Aug 15, 2025

I think it's a good idea as we already have it in dependencies.

  • Refactor test-main.ts to use Jest's built-in test features
  • Use Jest's describe(), test(), and test.each() for test orchestration
  • Replace custom ValidationResults with Jest's expect() assertions
  • Maintain all original functionality

The refactored code leverages Jest's built-in capabilities for test execution, result aggregation, and error reporting instead of reimplementing these features in custom code.

Amp-Thread-ID: https://ampcode.com/threads/T-e2754756-4d6f-4a0d-8f3f-125ca145202c

- Refactor test-main.ts to use Jest's built-in test features
- Use Jest's describe(), test(), and test.each() for test orchestration
- Replace custom ValidationResults with Jest's expect() assertions
- Maintain all original functionality

The refactored code leverages Jest's built-in capabilities for test
execution, result aggregation, and error reporting instead of
reimplementing these features in custom code.

Co-authored-by: Amp <amp@ampcode.com>
Amp-Thread-ID: https://ampcode.com/threads/T-e2754756-4d6f-4a0d-8f3f-125ca145202c
@jupblb jupblb requested a review from varungandhi-src August 15, 2025 13:05
@jupblb jupblb self-assigned this Aug 15, 2025
jupblb added 2 commits August 15, 2025 15:12
- Add isJest flag to detect runtime environment
- Replace Jest expect() calls with conditional logic
- Provide standalone implementation for non-Jest execution
- Maintain backwards compatibility with npm run check-snapshots
Copy link
Contributor

@varungandhi-src varungandhi-src left a comment

Choose a reason for hiding this comment

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

Nice simplification. I think this is not behavior equivalent though -- this change is removing the recording for the test names for the sometimes assertions; the point of that was to enable someone to quickly debug which test sets a particular condition a certain way without having to make the test fail/use a debugger.

Overall, I'm not a big fan of outsourcing the handling of the test "main" function to some other tool. This means yet another tool to learn (although, one could reasonably argue that this test's harness's set of supported flags is also ad-hoc), and it makes it difficult to follow the basic logic of how tests get invoked. I just checked -- jest is currently at major version 30, which doesn't inspire confidence in terms of stability.

We should at least update the CONTRIBUTING.md if any changes are needed there; I'm assuming jest has a different set of flags.

Will take a closer look on Monday if you're still keen on using jest; I don't have a strong opinion on it.

jupblb added 3 commits August 15, 2025 15:21
- Update Jest configuration to include test directory
- Create tsconfig.test.json for test-specific TypeScript config
- Remove webpack bundling of test files
- Update package.json scripts to run tests through Jest
- Add module mappings for pyright-internal dependencies
- Remove dual-mode execution from test-main.ts
- Update GitHub workflow to not require pre-build for tests
- Add separate jest.snapshot.config.js for snapshot tests
- Configure module mappings for pyright-internal dependencies
- Disable TypeScript diagnostics in Jest to avoid compilation errors
- Ensure build runs before snapshot tests in GitHub workflow
- Clean up unused run-tests.js file
@jupblb
Copy link
Member Author

jupblb commented Aug 15, 2025

Thanks for taking a look. This PR is more me playing with the code than a solid proposal. Feel free to reject this.

I did decide to use Jest here only because I saw it in the package.json dependencies. There is no existing test that uses it. So, alternatively to this PR, we can just remove it.

@jupblb
Copy link
Member Author

jupblb commented Aug 15, 2025

Actually, I got into more issues with this test runner. I think it's better to drop it for now. I'm closing this PR for now.

@jupblb jupblb closed this Aug 15, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants