Skip to content

chore: add type checking and linting for tests#435

Merged
wrslatz merged 2 commits intomainfrom
issue/411
Apr 15, 2026
Merged

chore: add type checking and linting for tests#435
wrslatz merged 2 commits intomainfrom
issue/411

Conversation

@wrslatz
Copy link
Copy Markdown
Contributor

@wrslatz wrslatz commented Apr 15, 2026

Summary

This PR implements the changes requested in #411 to add type checking and linting for test files.

Changes

  • tsconfig.json: Include test directory for development with strict type checking
  • tsconfig.build.json (new): Extends tsconfig.json but excludes tests and node_modules for production builds
  • next.config.mjs: Configure Next.js to use tsconfig.build.json via tsconfigPath property
  • eslint.config.mjs: Uncomment and enable test linting with Jest plugin configuration
  • package.json: Add eslint-plugin-jest (v29.15.2) dependency
  • Test file fixes: Resolved all TypeScript and ESLint errors in test files

Architecture Benefits

Development: Tests included in type checking → catches bugs early
Production builds: Tests excluded via tsconfig.build.json
Code quality: Tests have same type safety and linting as production code
Clean separation: Main tsconfig for dev/linting, tsconfig.build for builds

Testing

  • ✅ All 19 tests pass
  • ✅ Type checking passes (both tsconfig.json and tsconfig.build.json)
  • ✅ ESLint passes (no errors in test files)
  • ✅ Build succeeds

Fixes #411

- Include test directory in tsconfig.json for development with strict type checking
- Create tsconfig.build.json that extends tsconfig.json but excludes tests and node_modules
- Configure Next.js to use tsconfig.build.json for production builds via tsconfigPath property
- Uncomment and enable test linting in eslint.config.mjs
- Add eslint-plugin-jest to package.json for Jest-specific linting rules
- Fix TypeScript errors in test files:
  - test/server/git/controller.test.ts: Cast fakeOctokitData to SyncReposSchema type
  - test/app.ts: Refactor to return Promise instead of using done callback
- Fix ESLint errors in test files:
  - test/octomock/index.ts: Add disable comment for mock-specific any types, fix empty object types
  - test/app.test.ts: Replace any types with proper type assertions
  - test/app.ts: Replace any types with proper type assertions
- All tests pass and build succeeds with both tsconfig configurations

Fixes #411

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@wrslatz wrslatz requested review from a team as code owners April 15, 2026 18:14
@wrslatz wrslatz requested review from Miablo and riley-kohler April 15, 2026 18:23
@wrslatz
Copy link
Copy Markdown
Contributor Author

wrslatz commented Apr 15, 2026

Open questions:

  1. Should we add TypeScript checks (tsc --no-emit) as part of CI to catch type errors not caught by the Next.js build?
  2. Should we split formatting (Prettier) from linting to make it clearer which is failing, both in scripts and in CI? If others agree, I will do that in a follow-on PR.

@riley-kohler
Copy link
Copy Markdown
Contributor

Addressing your open questions before I fully review:

  1. Should we add TypeScript checks (tsc --no-emit) as part of CI to catch type errors not caught by the Next.js build?

I'm all for type checking being added, it's one of the main benefits of the language so we should make use of it and lift the code base up to that higher bar.

  1. Should we split formatting (Prettier) from linting to make it clearer which is failing, both in scripts and in CI? If others agree, I will do that in a follow-on PR.

I'm fine with splitting them or leaving them together. It's a pretty minor change in my eyes.

@wrslatz
Copy link
Copy Markdown
Contributor Author

wrslatz commented Apr 15, 2026

Addressing your open questions before I fully review:

  1. Should we add TypeScript checks (tsc --no-emit) as part of CI to catch type errors not caught by the Next.js build?

I'm all for type checking being added, it's one of the main benefits of the language so we should make use of it and lift the code base up to that higher bar.

@riley-kohler Do you think that should be added in this PR or a follow-on? And should it go in the build workflow or somewhere else?

@wrslatz
Copy link
Copy Markdown
Contributor Author

wrslatz commented Apr 15, 2026

@riley-kohler Do you think that should be added in this PR or a follow-on? And should it go in the build workflow or somewhere else?

I'll probably add a separate workflow and do it in a follow-on alongside the lint and format split change, since we'll need required statuses updated with both anyway.

Copy link
Copy Markdown
Contributor

@riley-kohler riley-kohler left a comment

Choose a reason for hiding this comment

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

Looks good!

@riley-kohler
Copy link
Copy Markdown
Contributor

All of the open question changes sound like good follow ons to me. I think type checking should just be added to the npm build script so that it always runs locally and in the build workflow. Same for lint/format, I think the CI checks should just mirror the local commands.

@wrslatz wrslatz merged commit a21987d into main Apr 15, 2026
10 checks passed
@wrslatz wrslatz deleted the issue/411 branch April 15, 2026 18:56
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.

Add type checking and linting for tests

2 participants