Skip to content

Conversation

jakebailey
Copy link
Member

@jakebailey jakebailey commented Mar 14, 2025

  • internal/testutil/runner -> internal/testrunner
  • TestCompilerBaselinesLocal -> TestLocal
  • TestCompilerBaselinesSubmodule -> TestSubmodule
  • Tests like TestCompilerBaselinesSubmodule/conformance/globalThisUnknownNoImplicitAny/error are now TestSubmodule/globalThisUnknownNoImplicitAny/error.
    • Our test names never have duplicates, so this is safe.

These should make it simpler to make a generic launch task like we do in Strada.

@jakebailey
Copy link
Member Author

Notably this does not move baselines around in testdata, at least not yet. I could tack that onto this PR but I think it might be prudent to do that in separate massive PR.

@gabritto
Copy link
Member

gabritto commented Mar 14, 2025

Tests like TestCompilerBaselinesSubmodule/conformance/globalThisUnknownNoImplicitAny/error are now TestSubmodule/globalThisUnknownNoImplicitAny/error.

  • Our test names never have duplicates, so this is safe.

This may be true now, but if we want to keep that invariant in the future, we also need to port the code that checks for duplicates, which I intentionally did not port before.
Also, I copied a couple tests from Strada to the local Corsa tests, so those will be duplicated in the local and submodule tests, and need to be removed.

@jakebailey
Copy link
Member Author

This may be true now, but if we want to keep that invariant in the future, we also need to port the code that checks for duplicates, which I intentionally did not port before.

Ah, yes, we should make a test which verifies this explicitly.

Also, I copied a couple tests from Strada to the local Corsa tests, so those will be duplicated in the local and submodule tests, and need to be removed.

Those are still separate; it's TestLocal and TestSubmodule, they're not merged together or anything.

@jakebailey jakebailey marked this pull request as draft March 14, 2025 00:42
@jakebailey jakebailey marked this pull request as ready for review March 14, 2025 02:56
Copy link
Member

@gabritto gabritto left a comment

Choose a reason for hiding this comment

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

I don't know if it will in fact matter, but the idea is that we'll have more test runners (e.g. emit runner) when we port more of the test infra. Then deleting that Runner interface might interfere with running all the runners in a single Go test.

@jakebailey
Copy link
Member Author

Are those tests also in the same namespace (in terms of test name)? I can revert that change for sure.

@jakebailey jakebailey enabled auto-merge March 14, 2025 17:48
@jakebailey jakebailey disabled auto-merge March 14, 2025 18:01
@jakebailey
Copy link
Member Author

Actually, I just added to the PR a launch task example. You just have to open a test in the submodule dir, and then it works.

@jakebailey jakebailey enabled auto-merge March 14, 2025 18:02
@jakebailey jakebailey added this pull request to the merge queue Mar 14, 2025
Merged via the queue into main with commit 53ee499 Mar 14, 2025
21 checks passed
@jakebailey jakebailey deleted the jabaile/test-refactor branch March 14, 2025 18:20
zshannon pushed a commit to zshannon/typescript-go that referenced this pull request Oct 6, 2025
zshannon pushed a commit to zshannon/typescript-go that referenced this pull request Oct 6, 2025
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.

2 participants