Skip to content

Conversation

@jakebailey
Copy link
Member

I think these were valuable early on when we didn't have the rest of the test infra, but I'm not sure they are benefiting us now (and the fixtures aren't up to date). Do we need these anymore?

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR removes the entire internal/module/resolver_test.go file, which contained module resolution tests that used a JSON fixture-based approach (resolvertests.json). The test file included a VFS-based module resolution host implementation and tested module name resolution, type reference directive resolution, and package scope lookup with both sequential and concurrent execution patterns.

Key Changes

  • Removed 398 lines of test code for module resolver functionality
  • Removed VFS-based test infrastructure for module resolution testing
  • Removed baseline/snapshot testing for module resolution traces
Comments suppressed due to low confidence (1)

internal/module/resolver_test.go:1

  • Removing this entire test file eliminates all direct unit tests for the module resolver. The module resolution functionality in internal/module/resolver.go (~2000 lines) will have no dedicated test coverage. According to the coding guidelines, when fixing bugs or implementing features, at least one minimal test case should be added. Before removing these tests, equivalent test coverage should be established, either through compiler tests in testdata/tests/cases/compiler/ or alternative unit tests.

@jakebailey jakebailey added this pull request to the merge queue Nov 4, 2025
Merged via the queue into main with commit 7f832a8 Nov 4, 2025
22 checks passed
@jakebailey jakebailey deleted the jabaile/remove-resolver-tests branch November 4, 2025 16:46
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.

4 participants