Adding baseline methods for verification of findAllRefs, GoTo*, Occurrences, highlights and rename deprecating the corresponding verification methods from fourslash tests

[sheetalkamat]

This baselines all the tests that have context span, working on top of existing baseline infra for findAllRefs, just making it easier to reason about context spans and results.
Consolidated all the baseline tests so they can be worked together to create single baseline..
Most of this was find and replace with minor manual consolidation.

With this change for testing purposes use:

• verify.baselineCommands - when needing to baseline multiple commands like find all references, document hightlights etc, rest of the methods are individual command baseline
• verify.baselineGetFileReferences
• verify.baselineFindAllReferences
  Deprecated:
  • verify.singleReferenceGroup
• verify.baselineGoToDefinition - to test getDefinitionAndBoundSpan
  Deprecated:
  • verify.goToDefinition
• verify.baselineGetDefinitionAtPosition - to test getDefinitionAtPosition
  Deprecated:
  • verify.goToDefinitionIs
  • verify.goToDefinitionForMarkers
  • verify.goToDefinitionName
• verify.baselineGoToSourceDefinition
  Deprecated:
  • verify.goToSourceDefinition
• verify.baselineGoToType
  Deprecated:
  • verify.goToType
  • verify.typeDefinitionCountIs
• verify.baselineGoToImplementation
  Deprecated:
  • verify.allRangesAppearInImplementationList
  • verify.implementationListIsEmpty
  • goTo.implementation
• verify.baselineOccurences
  Deprecated:
  • verify.rangesAreOccurrences
  • verify.occurrencesAtPositionContains
  • verify.occurrencesAtPositionCount
• verify.baselineDocumentHighlights
  Deprecated:
  • verify.rangesAreDocumentHighlights
  • verify.rangesWithSameTextAreDocumentHighlights
  • verify.documentHighlightsOf
• verify.baselineRename
  Deprecated:
  • verify.rangesWithSameTextAreRenameLocations
  • verify.rangesAreRenameLocations
  • verify.renameLocations

[jakebailey]

The baselines before were seemingly entirely jsonc and now they are some other format; can we just keep it as jsonc so the file extension and highlighting are consistent?

[sheetalkamat]

The baselines before were seemingly entirely jsonc and now they are some other format; can we just keep it as jsonc so the file extension and highlighting are consistent?

Done now

[jakebailey]

So to be clear, this PR is actually mainly removing the ad-hoc verification methods and turning all of the fourslash tests into baselines, correct?

(If so, we should retitle the PR to say that just for the git history.)

[sheetalkamat]

I will list, methods that we are retiring and adding baselines for. (anyThing that has contextSpan (findAllRefs, goTo*, rename, occurences and highlights)

[jakebailey]

One thing I'm grappling with here is that it seems like all of this info here is a duplicate of the information that's already in these baslines beforehand; is it possible to only add the new info instead of duplicating the display parts and such instead? It seems like that's a lot of the source of the huge amount of added lines.

[sheetalkamat]

It’s Some of the info that tests were checking as part of range additional data .. now I feel like it’s one blob if it’s json and so hard to decipher so I added it here to make easier to read .. contemplated removing json part all together but we would miss additional data properties when added so I let it be .. not sure what’s better approach here

[sheetalkamat]

Eg there are some tests that check display parts , most check write acces in references etc

[andrewbranch]

My main objection is the sheer quantity of additional text this brings. I find it really hard to look through the new format and understand what’s being presented. If the primary goal is just to show context spans, we could try an underlining scheme instead of using [| range markers; for example, going from the current go-to-def format:

// context context [|actual range|] context context

to

// context context actual range context context
// ┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄━━━━━━━━━━━━┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄

But repeating the whole file text (or even slices of it) seems pretty undesirable.

[sheetalkamat]

// context context actual range context context
// ┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄━━━━━━━━━━━━┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄

This becomes pretty tricky with overlapping spans and context only for partial range. Eg.

class D { static someMethod[] { D.xxx } } and now the context is only for definition of D but not for reference of D but both are in the context range since context is complete declaration span for the class.

We definitely need readable context as its completely lost. I can change it do the suggested change in contextSpan higlight to remove one additional text slice.

Also was thinking of removing the JSON dump since that is not readable at all. Instead asserting that the result doesnt contain anything other than properties part of readable baseline.

[andrewbranch]

Another option would be to report the context span in an abbreviated way, noting the line range and first and last characters:

// Context span: L23-35 `class Foo ... } }`

[sheetalkamat]

Here is compact way the baseline is working now:

The additions are that it inlines contextSpan with <| and |> and writes the index of the context span as contextId for the textSpan and other additional data between {| and |} as textStart marker. This is how tests were written except context was marked with same range but this simplifies identifying context vs spans.

Find all refs is special because it returns definitions and references so definition index is coded as part of reference and definition and they are not mixed because it just gets so confusing. The Definitions list is printed separately just like references grouped by file names so its not writing file contents again and again if two definition fall in same file.

All the baselines have same format so hopefullu that should make it consistent and easier to understand between different types of commands.

// === /a.ts ===
// <|export default function /*FIND ALL REFS*/[|{| contextId: 0, defId: 0, isWriteAccess: true, isDefinition: true |}a|]() {}|>

// === /b.ts ===
// <|import [|{| contextId: 1, defId: 1, isWriteAccess: true |}a|], * as ns from "./a";|>

// === Definitions ===
// === /a.ts ===
// <|export default function /*FIND ALL REFS*/[|{| contextId: 0, defId: 0, kind: "function", name: "function a(): void", displayParts: [{"text":"function","kind":"keyword"},{"text":" ","kind":"space"},{"text":"a","kind":"functionName"},{"text":"(","kind":"punctuation"},{"text":")","kind":"punctuation"},{"text":":","kind":"punctuation"},{"text":" ","kind":"space"},{"text":"void","kind":"keyword"}] |}a|]() {}|>

// === /b.ts ===
// <|import [|{| contextId: 1, defId: 1, kind: "alias", name: "(alias) function a(): void\nimport a", displayParts: [{"text":"(","kind":"punctuation"},{"text":"alias","kind":"text"},{"text":")","kind":"punctuation"},{"text":" ","kind":"space"},{"text":"function","kind":"keyword"},{"text":" ","kind":"space"},{"text":"a","kind":"aliasName"},{"text":"(","kind":"punctuation"},{"text":")","kind":"punctuation"},{"text":":","kind":"punctuation"},{"text":" ","kind":"space"},{"text":"void","kind":"keyword"},{"text":"\n","kind":"lineBreak"},{"text":"import","kind":"keyword"},{"text":" ","kind":"space"},{"text":"a","kind":"aliasName"}] |}a|], * as ns from "./a";|>

// NOTE JSON was already being emitted
[
  {
    "definition": {
      "containerKind": "",
      "containerName": "",
      "fileName": "/a.ts",
      "kind": "function",
      "name": "function a(): void",
      "textSpan": {
        "start": 24,
        "length": 1
      },
      "displayParts": [
        {
          "text": "function",
          "kind": "keyword"
        },
        {
          "text": " ",
          "kind": "space"
        },
        {
          "text": "a",
          "kind": "functionName"
        },
        {
          "text": "(",
          "kind": "punctuation"
        },
        {
          "text": ")",
          "kind": "punctuation"
        },
        {
          "text": ":",
          "kind": "punctuation"
        },
        {
          "text": " ",
          "kind": "space"
        },
        {
          "text": "void",
          "kind": "keyword"
        }
      ],
      "contextSpan": {
        "start": 0,
        "length": 30
      }
    },
    "references": [
      {
        "textSpan": {
          "start": 24,
          "length": 1
        },
        "fileName": "/a.ts",
        "contextSpan": {
          "start": 0,
          "length": 30
        },
        "isWriteAccess": true,
        "isDefinition": true
      }
    ]
  },
  {
    "definition": {
      "containerKind": "",
      "containerName": "",
      "fileName": "/b.ts",
      "kind": "alias",
      "name": "(alias) function a(): void\nimport a",
      "textSpan": {
        "start": 7,
        "length": 1
      },
      "displayParts": [
        {
          "text": "(",
          "kind": "punctuation"
        },
        {
          "text": "alias",
          "kind": "text"
        },
        {
          "text": ")",
          "kind": "punctuation"
        },
        {
          "text": " ",
          "kind": "space"
        },
        {
          "text": "function",
          "kind": "keyword"
        },
        {
          "text": " ",
          "kind": "space"
        },
        {
          "text": "a",
          "kind": "aliasName"
        },
        {
          "text": "(",
          "kind": "punctuation"
        },
        {
          "text": ")",
          "kind": "punctuation"
        },
        {
          "text": ":",
          "kind": "punctuation"
        },
        {
          "text": " ",
          "kind": "space"
        },
        {
          "text": "void",
          "kind": "keyword"
        },
        {
          "text": "\n",
          "kind": "lineBreak"
        },
        {
          "text": "import",
          "kind": "keyword"
        },
        {
          "text": " ",
          "kind": "space"
        },
        {
          "text": "a",
          "kind": "aliasName"
        }
      ],
      "contextSpan": {
        "start": 0,
        "length": 29
      }
    },
    "references": [
      {
        "textSpan": {
          "start": 7,
          "length": 1
        },
        "fileName": "/b.ts",
        "contextSpan": {
          "start": 0,
          "length": 29
        },
        "isWriteAccess": true,
        "isDefinition": false
      }
    ]
  }
]

Before this PR, rename baselines would rewrite file text with content between textSpan start and end with RENAME and mark originating location as [|RENAME|]. Now it uses same baselining format as find all references (that is location of invoke is marked as /*RENAME*/ and ranges and context spans marked accordingly. But it also adds RENAME suffix to text so its easier to check how things will be renamed just like before. It also writes suffix and prefix text that result rewritten.

// BEFORE
/*====== /src/index.ts ======*/

import * as [|RENAME|] from '../lib/index';
RENAME.someExportedVariable;

// After
// === RenameOptions ===
// providePrefixAndSuffixTextForRename: true
// === /src/index.ts ===
// <|import { /*START PREFIX*/someExportedVariable as /*RENAME*/[|{| contextId: 0 |}someExportedVariableRENAME|] } from '../lib/index';|>
// [|someExportedVariableRENAME|];

[
  {
    "textSpan": {
      "start": 9,
      "length": 20
    },
    "fileName": "/src/index.ts",
    "contextSpan": {
      "start": 0,
      "length": 52
    },
    "prefixText": "someExportedVariable as "
  },
  {
    "textSpan": {
      "start": 53,
      "length": 20
    },
    "fileName": "/src/index.ts"
  }
]

[andrewbranch]

// <|import [|{| contextId: 1, defId: 1, kind: "alias", name: "(alias) function a(): void\nimport a", displayParts: [{"text":"(","kind":"punctuation"},{"text":"alias","kind":"text"},{"text":")","kind":"punctuation"},{"text":" ","kind":"space"},{"text":"function","kind":"keyword"},{"text":" ","kind":"space"},{"text":"a","kind":"aliasName"},{"text":"(","kind":"punctuation"},{"text":")","kind":"punctuation"},{"text":":","kind":"punctuation"},{"text":" ","kind":"space"},{"text":"void","kind":"keyword"},{"text":"\n","kind":"lineBreak"},{"text":"import","kind":"keyword"},{"text":" ","kind":"space"},{"text":"a","kind":"aliasName"}] |}a|], * as ns from "./a";|>

This isn’t human-readable, so I don’t really think making it a baseline gains anything over the (also non-human-readable) old fourslash tests. If I generated one of these baselines when working on a new feature, I would have no idea if it was correct and would probably just test my changes in VS Code and then assume the baselines are right without really reading them.

The text-based baselines tend to be incomplete, which is why they’re usually paired with JSON. The text should provide you with a high-level correctness check at a glance, while the JSON protects against regressions that wouldn’t manifest in the text. If the text can be cleverly improved to show more, I think that’s great, but if it becomes less readable by duplicating information in the JSON, that’s purely a loss. If there are text baselines without JSON included, it seems reasonable to add it. We can also manipulate the JSON to make it a bit more readable too, like substituting text spans with actual string slices and concatenating displayParts together.

[sheetalkamat]

The part that’s not relatable with json is that it’s so hard to figure out what span it’s talking about .. not all tests are easy to check with vscode esp dealing with big changes .. the part that’s not readable is because of huge display parts rather than anything else .. eg look at document highlights baselines they are much more readable now in my opinion ..

find all references is a tricky business and there were tests that tested display parts that were not better than current .. having said that we can experiment with json substitution for the display parts but I would like to keep context span thing as is .. that was well tested and covered when added but lost when we converted these tests to baselines making it not comprehensible at all ..

[andrewbranch]

Is there any hope of splitting this PR up? It seems that every command’s baseline is subtly different, and I’ve only made it into the ones that start with d, but GitHub keeps going non-responsive and crashing as I try to review.

[sheetalkamat]

@andrewbranch I had to force push since I used my WIP branch instead and ran tests on each commit. Hopefully this helps. It took a while to run the tests on all these commits.

[andrewbranch]

@typescript-bot cherry-pick this to release 5.0 because another cherry-picked PR depends on it

[typescript-bot]

Heya @andrewbranch, I've started to run the task to cherry-pick this into release on this PR at f837b62. You can monitor the build here.

[typescript-bot]

Hey @andrewbranch, I couldn't open a PR with the cherry-pick. (You can check the log here). You may need to squash and pick this PR into release manually.