Update test callable discovery to work within the language service #2095

sezna · 2025-01-07T19:38:17Z

This PR rewrites #2059 to trigger test discovery from within the language server, as opposed to externally in an entirely separate context. Please read the description in that PR for more detail.

It also adds tests for LS state in both the Rust-based LS tests and the JS-based basics.js npm API tests.

…lex/track_test_changes_in_ls

minestarks

Not done looking at all the files, posting all my comments for today.

compiler/qsc/src/lib.rs

wasm/src/test_discovery.rs

minestarks · 2025-01-08T01:09:59Z

wasm/src/language_service.rs

+
+#[wasm_bindgen]
+extern "C" {
+    #[wasm_bindgen(typescript_type = "(callables: [string, ILocation][]) => void")]


Use the ITestCallable type you already defined in the other wasm file, instead of the [string, ILocation] tuple. Tuples are clunky in TS.

npm/qsharp/src/language-service/language-service.ts

npm/qsharp/src/compiler/compiler.ts

wasm/src/language_service.rs

minestarks

Finished looking. I really like the overall approach. Also I assume integration tests are coming up in a different PR.

minestarks · 2025-01-08T20:16:03Z

compiler/qsc_passes/src/test_attribute.rs

+                        decl.name.span,
+                    ));
+            }
+            if decl.input.ty != qsc_hir::ty::Ty::UNIT {
+                self.errors
+                    .push(TestAttributeError::CallableHasParameters(decl.name.span));


Instead of just the name, could you use the whole signature's span, so that the squiggles appear below the problematic code and not the name?

And now that I wrote that... I realize there may not be an easy way to get that span from the AST. I think I hit this issue before. Hmm...

minestarks · 2025-01-08T20:31:57Z

compiler/qsc_passes/src/test_attribute/tests.rs

As it was brought up in the team chat, I think it'd be useful to have a test case where a @Config() attribute causes a test callable to be excluded from the compilation. get_test_callables shouldn't return that callable.

minestarks · 2025-01-08T20:39:19Z

language_service/src/state.rs

+                        // TODO(sezna) verify encoding
+                        crate::qsc_utils::into_location(
+                            qsc::line_column::Encoding::Utf16,


For consistency, this should be the same position_encoding that the LanguageService was initialized with. You can add it as a parameter to CompilationStateUpdater::new and just keep it in this struct.

minestarks · 2025-01-08T20:55:37Z

language_service/src/state/tests.rs

+    // Trigger a document update for the second test file.
+    updater
+        .update_document(
+            "parent/src/test2.qs",
+            1,
+            "@Test() function Test2() : Unit {}",
+        )
+        .await;


It'd be interesting to test if the tests in test2.qs file get discovered WITHOUT triggering an update for test2.qs.

In theory, just poking test1.qs should cause the whole project to be loaded, which in turn will raise a notification about all the tests.

If you remove this line, I think you'll see in the below expect! that all the tests are still discovered. (Currently, the expect! output contains TWO notifications with the same contents, because the compilation is updated twice. After you remove the second update_document, the expect! output should contain ONE notification, which will still show all the tests).

I suggest just removing the second update. It'll make the test case both shorter and more "interesting".

minestarks · 2025-01-08T21:12:23Z

vscode/src/language-service/activate.ts

@@ -37,15 +37,25 @@ import { registerQSharpNotebookCellUpdateHandlers } from "./notebook.js";
 import { createReferenceProvider } from "./references.js";
 import { createRenameProvider } from "./rename.js";
 import { createSignatureHelpProvider } from "./signature.js";
-
-export async function activateLanguageService(extensionUri: vscode.Uri) {


FWIW I had intentionally made this function take extensionUri instead of the whole context, since we never actually use anything but the extensionUri. I prefer the previous version.

minestarks · 2025-01-08T21:15:10Z

vscode/src/language-service/testExplorer.ts

+  const compilerWorkerScriptPath = vscode.Uri.joinPath(
+    context.extensionUri,
+    "./out/compilerWorker.js",
+  ).toString();
+  worker = getCompilerWorker(compilerWorkerScriptPath);


I'd love it if you pulled this into a common utility in vscode, since these two lines, including the hardcoded path, are repeated in 4-5 places now.

minestarks · 2025-01-09T00:28:41Z

library/signed/src/Tests.qs

+@Test()
+operation MeasureSignedIntTests() : Unit {
+    let testCases = [
+        ("0b0001 == 1", 4, (qs) => X(qs[0]), (qs) => MeasureSignedInteger(qs, 6), 11),


This doesn't actually pass for me .

minestarks · 2025-01-09T00:29:21Z

vscode/src/language-service/testExplorer.ts

+    false,
+  );
+
+  const testMetadata = new WeakMap<vscode.TestItem, number>();


Interesting, I don't think I've ever seen a WeakMap used in practice! Is the idea that the test controller owns these items, and not this map?

That's from the docs. Search on weakmap on https://code.visualstudio.com/api/extension-guides/testing

This idea came from this page: https://code.visualstudio.com/api/extension-guides/testing (search for weakmap) -- to be honest, I'm not 100% sure why they recommend a WeakMap versus a regular key/value store here.

to be honest, I'm not 100% sure why they recommend a WeakMap versus a regular key/value store here

Because a regular key/value store would hold a strong reference to the TestItem that is the key you put in it, so it would never get garbage collected (unless you manually scrubbed the map and removed items which no longer exist in the controller, which you don't in this code currently. You delete no longer needed items in line 182, but you don't remove them from the map, so they will remain held in memory by that reference if not using a WeakMap).

From the MDN docs: an object's presence as a key in a WeakMap does not prevent the object from being garbage collected. Once an object used as a key has been collected, its corresponding values in any WeakMap become candidates for garbage collection as well

minestarks · 2025-01-09T00:34:00Z

library/signed/src/Tests.qs

In addition to the tests here I'd love it if you added a samples under samples/ showing how to use this new attribute.

minestarks · 2025-01-09T00:35:00Z

language_service/src/protocol.rs

+#[derive(Debug)]
+pub struct TestCallables {
+    pub callables: Vec<(String, Location)>,
+    pub version: Option<u32>,


Unused field (and rightly so!)

minestarks · 2025-01-09T00:46:57Z

@sezna hitting this odd bug where you can't navigate to the test case after it fails.

language_service/src/state.rs

+                .map(|(name, span)| {
+                    (
+                        name.clone(),
+                        // TODO(sezna) verify encoding


sezna and others added 30 commits December 10, 2024 06:04

initial work on testing

b79c31e

Merge branch 'main' of github.com:microsoft/qsharp into alex/testHarness

1c09783

progress on test explorer

54eb209

test collection works

e6a0941

tests run

0495a6d

add todos

eae1b4d

switching to namespaces included with callable names

759036f

scoped test names

cec4bf9

deduplicate parent items

ad7d3e2

make running child items work

d9f288d

auto-refresh test cases

9ec9f41

wip -- checkpoint

a03ed14

wip

c11476a

Remove codelens stuff

b247c35

update libraries to use new testing harness

e75f65b

remove bad imports

131a341

document some functions

b52ad25

Add comment

180368e

Remove todos

e960916

Fix nested tests

7d2aadc

initial round of PR feedback

ac8bfd7

move discovery of test items into compiler layer

c22d14d

Use a pass to detect test attribute errors and report them nicely

5a28ef1

use getActiveProgram

16a2aed

remove unnecessary api in main.ts

214097c

Fmt

adbc4b2

fix lints

5b6a1f8

update tests

7cee532

filter out invalid test items

be64105

Merge branch 'main' of github.com:microsoft/qsharp into alex/testHarness

e34b7d2

sezna added 13 commits January 2, 2025 14:26

Remove comment

cf35b19

wip

36abbd1

wip

bf8b277

individual test parity tracking works

ff31000

Rename from parity to version

b72ca75

pull test explorer out into its own file

f52330d

lint fixes

865cce1

rust lint updates

1f88c97

update expect test

e0347c6

update test case for new test callables

468345b

basics.js test

00d5f4f

add basics.js tests

57bc6c1

undo common compiler worker

d1267c4

sezna marked this pull request as ready for review January 7, 2025 20:37

sezna requested review from billti, swernli, idavis, minestarks and ScottCarda-MS as code owners January 7, 2025 20:37

sezna added 2 commits January 7, 2025 12:48

Merge branch 'main' of github.com:microsoft/qsharp into alex/testHarness

8255955

Merge branch 'alex/testHarness' of github.com:microsoft/qsharp into a…

007a76d

…lex/track_test_changes_in_ls

sezna changed the base branch from alex/testHarness to main January 7, 2025 23:41

sezna requested a review from DmitryVasilevsky as a code owner January 7, 2025 23:41

minestarks reviewed Jan 8, 2025

View reviewed changes

idavis mentioned this pull request Jan 8, 2025

Test Explorer in VS Code Extension and @Test() Attribute #2059

Closed

minestarks requested changes Jan 9, 2025

View reviewed changes

Fix output

1d5ef8e

github-advanced-security bot found potential problems Jan 9, 2025

View reviewed changes

language_service/src/state.rs

.map(|(name, span)| {

(

name.clone(),

// TODO(sezna) verify encoding

Check notice

Code scanning / devskim

A "TODO" or similar was left in source code, possibly indicating incomplete functionality Note

Suspicious comment

mid PR review feedback

6ca81d0

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update test callable discovery to work within the language service #2095

Update test callable discovery to work within the language service #2095

sezna commented Jan 7, 2025 •

edited

Loading

minestarks left a comment

minestarks Jan 8, 2025

minestarks left a comment

minestarks Jan 8, 2025

minestarks Jan 8, 2025

minestarks Jan 8, 2025

minestarks Jan 8, 2025

minestarks Jan 8, 2025

minestarks Jan 8, 2025

minestarks Jan 9, 2025

minestarks Jan 9, 2025

billti Jan 9, 2025

sezna Jan 9, 2025

billti Jan 9, 2025

minestarks Jan 9, 2025

minestarks Jan 9, 2025

minestarks commented Jan 9, 2025

Update test callable discovery to work within the language service #2095

Are you sure you want to change the base?

Update test callable discovery to work within the language service #2095

Conversation

sezna commented Jan 7, 2025 • edited Loading

minestarks left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

minestarks left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

minestarks commented Jan 9, 2025

sezna commented Jan 7, 2025 •

edited

Loading