test(semantic): add comprehensive regression test suite

[DonIsaac]

What This PR Does

Enhance's oxc_semantic's integration tests with a regression test suite that ensures semantic's contract guarantees hold over all test cases in typescript-eslint's scope snapshot tests. Each test case checks a separate assumption and runs independently from other test cases.

This PR sets up the code infrastructure for this test suite and adds two test cases to start us off:

1. Reflexivity tests for IdentifierReference and Reference
2. Symbol declaration reflexivity tests between declarations in SymbolTable and their corresponding node in the AST.

Please refer to the doc comments for each of these tests for an in-depth explanation.

Aren't our existing tests sufficient?

oxc_semantic is currently tested directly via

1. scope snapshot tests, ported from typescript-eslint
2. Hand-written tests using SemanticTester in tests/integration

And indirectly via

3. Conformance test suite over Test262/TypeScript/Babel
4. Linter snapshot tests

Shouldn't this be sufficient? I argue not, for two reasons:

1. Clarify Contract Ambiguity

When using Semantic, I often find myself asking these questions?

• Does semantic.symbols().get_declaration(id) point to a BindingIdentifer/BindingPattern or the declaration that holds an identifier/pattern?
• Will a Reference's node_id point me to an IdentifierReference or the expression/statement that is holding an IdentifierReference?
• When will BindingIdentifier's symbol_id get populated? can we guarantee that after semantic analysis it will never be None?
• What actually is the node covered by semantic.symbols().get_span(id)? This one really messed me up, and resulted in me creating Incorrect Span on BindingIdentifier  #4739.
• What scope does Function::scope_id point to? The one where the function is declared? The one created by its body? The one created by the type annotations but before the function body? Or something else entirely?

These test cases are meant to answer such questions and guarnatee those answers as test cases. No other existing testing solution currently upholds such promises: they only tell us if code expecting one answer or another produces an unexpected result. However, those parts of the codebase could always be adjusted to conform to new Semantic behavior, meaning no contract guarantees are actually upheld.

2. Existing Tests Do Not Test The Same Behavior

I'll cover each above listed test case one-by-one:

1. For starters, these tests only cover scopes. Additionally, they only tell us how behavior has changed, not that behavior is now incorrect.
2. These do generally cover the same behaviors, but are not comprehensive and are difficult to maintain. These are unit tests that should be used hand-in-hand with this new test suite.
3. The most relevant tests here are for the parser. However, these tests only tell us if a syntax/parse error was produced, and tell us nothing about the validity of Semantic.
4. Relying on lint rule's output is a a mediiocre proxy of Semantic's behavior at best. They can tell us if changes to Semantic break assumptions made by lint rules, but they do not tell us if **those assumptions are the ones we want to uphold to external crates consuming Semantic.

[graphite-app]

Your org has enabled the Graphite merge queue for merging into main

Add the label “0-merge” to the PR and Graphite will automatically add it to the merge queue when it’s ready to merge. Or use the label “hotfix” to add to the merge queue as a hot fix.

You must have a Graphite account and log in to Graphite in order to use the merge queue. Sign up using this link.

[DonIsaac]

• docs(semantic): document SymbolTable #5978
• test(semantic): add comprehensive regression test suite #5976 👈
• main

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @DonIsaac and the rest of your teammates on Graphite

[codspeed-hq]

CodSpeed Performance Report

Merging #5976 will not alter performance

_{Comparing don/09-22-test_semantic_add_comprehensive_regression_test_suite (93575cd) with main (d60ceb4)}

Summary

✅ 29 untouched benchmarks

[Boshen]

I'll let @overlookmotel to decide on this one.

[overlookmotel]

Thanks very much for writing up the rationale for this change so clearly. I totally agree with the need for a more structured approach to testing, for all the reasons you have given. And I too want to know what semantic.symbols().get_declaration(id) is meant to be pointing at!

What I'm not so keen on is storing conformance test results in each snapshot. I am worried that if details of test fails are hidden away there, they may never get fixed.

I can imagine a scenario like this: Someone makes a PR that breaks a test. They say "I'll fix that test in a follow-up". But they forget to do it, and because it's hidden away in one .snap file out of 290, no-one notices and the bug lives on unfixed forever.

So personally, I think it'd be preferable if all tests fails were listed in a single .snap.md file.

[DonIsaac]

So personally, I think it'd be preferable if all tests fails were listed in a single .snap.md file.

I've looked into this, and making this change would require modifying how scope snapshot tests are run. I'd rather make changes to those tests in a separate PR. Is this a hard blocker, or can it be deferred to an up-stack PR?

[overlookmotel]

Is this a hard blocker, or can it be deferred to an up-stack PR?

Not a blocker to merging, as far as I'm concerned. But personally I would like to see it happen!

[DonIsaac]

Is this a hard blocker, or can it be deferred to an up-stack PR?

Not a blocker to merging, as far as I'm concerned. But personally I would like to see it happen!

Ok, I'll definitely add this in a follow-up PR. Are there any other blockers I should address before we can merge this PR?

[graphite-app]

Merge activity

• Sep 24, 10:18 PM EDT: The merge label '0-merge' was detected. This PR will be added to the Graphite merge queue once it meets the requirements.
• Sep 24, 10:18 PM EDT: Boshen added this pull request to the Graphite merge queue.
• Sep 24, 10:35 PM EDT: Boshen merged this pull request with the Graphite merge queue.