-
Notifications
You must be signed in to change notification settings - Fork 583
Add compiler test runner #115
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
One thing I was envisioning here was that for all existing tests in the TS codebase, we don't acutally check in any test artifacts, but instead diff them against what we have here and check those diffs in; if the diff is missing (or, should be deleted) the test errors out. That way we can slowly pare things down to nothing until we get to parity. We can add new tests which contribute real baselines, which could be separate, though. It's not clear that we could do that without all of the other stuff ready, though, e.g. actual program construction. Perhaps that implies that this PR should be restructured to not actually reference |
Of course, I say that, but we still need to use |
0d75339
to
1842745
Compare
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
@@ -0,0 +1,430 @@ | |||
package harnessutil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couldn't put this in testutil
because it would cause a cycle with vfstest
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
testutil
itself is only meant for helpers for writing tests (e.g. an extension to assert
) and should be dependency free, so I think what you've done here is good. Potentially we should just move its contents to a package like assertx
or xassert
(not an uncommon naming convention to "extend" another package), but I'm not sure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Honestly to me this means that testutil
is actually the wrong place for any of this harness/runner stuff; I think I just didn't notice where it was going initially...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should I move runner
, this one, and baseline
to some other folder? Maybe "testharness".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think yes, but I don't think it has to be this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should be good; the errors and symbols baselines look good so far. I skimmed the rest and didn't notice anything else.
I'm mainly interested in the next stage, running our existing tests and diffing them, but that requires more code and tsconfig parsing, etc.
compilerBaselineRegex = regexp.MustCompile(`\.tsx?$`) | ||
requireRegex = regexp.MustCompile(`require\(`) | ||
referencesRegex = regexp.MustCompile(`reference\spath`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At some level I wonder if these can be dropped, given these seem like things that can be pretty easily found with regular string operations...
|
||
func NewCompilerBaselineRunner(testType CompilerTestType) *CompilerBaselineRunner { | ||
testSuitName := testType.String() | ||
basePath := "tests/cases/" + testSuitName |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this need to change, or is it a part of some other message we're expecting to keep/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I understand, why would it need to change? basePath
is the path within testdata
where we get the test files from.
This adds a compiler test runner, capable of running compiler tests with error baselines, which is the only kind of baseline we have so far.
Things missing:
Things temporary:
createProgram
yet, one that takes root files etc.I also have to move the tests to a
compiler_test
package or something, but I don't think the tests are useful now, so maybe I'll do that later.You can run specific compiler tests like so:
$ go test -run TestCompilerBaselines/.*/simpleTestSingleFile github.com/microsoft/typescript-go/internal/testutil/runner
The command for diffing baselines also has to change to something like
git diff --diff-filter=AM --no-index ./testdata/baselines/reference ./testdata/baselines/local