Implement basics of test API #755

kpodsiad · 2021-11-09T19:19:20Z

~~There are some compilation errors which will be fixed in #754~~ - done and merged

After the brainstorm, together with @tgodzik we concluded that using DAP Event to pass some structured data about test results (well-defined JSON) will be the best and the fastest approach.
For now, mentioned data is passed through an OutputEvent, more specifically as a testResult event which contains all necessary information about the whole suite and at the same time about every single test.

src/test-provider/output-analyzer.ts

src/test-provider/analyze-test-run.ts

tgodzik

Great work! I still have some questions, especially that I am not an expert in TS.

src/test-provider/util.ts

package.json

src/test-provider/analyze-test-run.ts

src/test-provider/types.ts

tgodzik · 2021-12-10T19:48:00Z

@gabro would you be able to take a look? Ignore me if you are busy. 😅

gabro · 2021-12-10T19:49:09Z

@tgodzik hey, I'm on vacation until tomorrow, but I can try taking a look on Monday!
In case it's blocking a release don't mind waiting for me :)

tgodzik · 2021-12-10T19:50:05Z

It's not blocking at all. I am just starting to think about the new release. Thanks!

kpodsiad · 2021-12-10T21:24:50Z

src/util.ts

+declare const sym: unique symbol;
+/**
+ * Creates a newtype without any runtime overhead. It's important for ID to be both unique and descriptive.
+ */
+export type newtype<A, ID extends string> = A & {
+  readonly [sym]: ID;
+};
+


I know it's a little bit magical to the person who is not familiar with the typescript type system & magic. However, these types created by newtype produce compile errors and have meaningful names instead of just string.

export type TargetName = newtype<string, "targetName">; export type TargetUri = newtype<string, "targetUri">; const x: TargetName = "app" as TargetName; const y: TargetUri = x;

Type 'TargetName' is not assignable to type 'TargetUri'. Type 'TargetName' is not assignable to type '{ readonly [sym]: "targetUri"; }'. Types of property '[sym]' are incompatible. Type '"targetName"' is not assignable to type '"targetUri"'.

It seems like it's too much for poor Tomek :D

I'm quite familiar with this encoding of newtypes, but I think the unique symbol should be the value, otherwise it's quite useless, the definition above is equivalent to:

export type newtype<A, ID extends string> = A & { readonly theType: ID; };

This way the uniqueness is only guaranteed by the string passed when defining the type, which can work ok, but you are not using the unique symbol.

An alternative encoding can just be:

export type Newtype<A, O> = A & O type UserId = Newtype<number, { readonly UserId: unique symbol }> type ThingId = Newtype<number, { readonly ThingId: unique symbol}>

Full example here:

https://www.typescriptlang.org/play?ssl=1&ssc=13&pln=1&pc=20#code/KYDwDg9gTgLgBDAnmYcBywDuSUB4CCANHAPIB8cAvHPnAGSkBQjOqAqgM7BQCSAJlXRZWuAHYBXALYAjbsQDecKMACGfCKIA2iOJ278AXHHGiAlgEdxqDohkRNcAL5kWyVABUAFqdEBzfoIY2G5iUrJQCkqq6lo6Xj7+fEYmFlZwNnaazsx8wADGmirKcABmJnkwphqlABTiXLxJug38AJRGAG4QpnwA3Iy5BUWoeRoc8PX6TXqN-YOFxaOi4wjefoZw8et9zCU1MGuJrYx7k42tQA

(this is loosely based on https://github.com/gcanti/newtype-ts/blob/master/src/index.ts)

Also (suuuper tiny detail) this is not strictly a newtype, since a TargetName can be passed where a string is expected. The encoding in newtype-ts is a bit stricter in this sense, that's why I've relaxed it a bit

I'm quite familiar with this encoding of newtypes, but I think the unique symbol should be the value, otherwise it's quite useless, the definition above is equivalent to:

Actually, I used that in this way intentionally. The symbol is declared as a unique one and is used as a key for unnamed property. Moreover, the symbol is not exported so only a newtype can use it. Thanks to that you cannot access this property or even fake it. What do I mean by "fake it"? Let's consider the following example:

type newtype<A, ID extends string> = A & { readonly theType: ID; }; type Type1 = newtype<string, "type1">; type Type2 = string & { theType: "type1"; }; const x: Type2 = "aezakmi" as Type2; const y: Type1 = x; // works

I know that this is an almost never gonna happen scenario, but I try to be careful with structural typing.

Also (suuuper tiny detail) this is not strictly a newtype, since a TargetName can be passed where a string is expected. The encoding in newtype-ts is a bit stricter in this sense, that's why I've relaxed it a bit

Yeah, you're right. But this is more than fine in this case since I wanted to use "newtype" to distinguish between string types and for us, the name newtype shouldn't be misleading.

Thanks for the explanation, it make sense then! One minor thing is that not using a unique symbol in the value too means that you can accidentally do:

type Type1 = newtype<string, "type1">; type Type2 = newtype<string, "type1">; // oops

and those two will be silently interchangeable.

It's probably ok if we don't have many cases, it's a mistake that can be caught in code review

src/test-provider/util.ts

gabro

Really nice job, I've left a first pass of stylistic comments

src/extension.ts

src/test-provider/analyze-test-run.ts

src/test-provider/test-manager.ts

gabro · 2021-12-13T16:01:04Z

src/test-provider/test-manager.ts

+      run.started(test);
+      const children = getTestItemChildren(test);
+      children.forEach((c) => run.enqueued(c));
+      await commands


I would prefer not mixing async/await and the then api of promises if possible.
Also in general this expression is very long and I think it would benefit in readability if broken down in multiple smaller expressions

Ok, so followng is a good way to go?

try { const debugSession = await commands... const wasStarted = await ... await analyzeResults() } catch(error) { }

tgodzik

Some questions from me, excuse me if they don't make sense 😅 I am a typescript amateur.

src/extension.ts

tgodzik · 2021-12-14T20:06:34Z

src/test-explorer/test-run-handler.ts

+  createDebugAdapterTracker(session) {
+    if (session.configuration.kind === testRunnerId) {
+      return {
+        onWillStartSession: () => testCache.setEmptySuiteResultsFor(session.id),


Can we run multiple tests at the same time?

It's a good point, some mechanism that prevents users from starting multiple test runs has to be added.

Actually, could we not allow it? Sorry for not wording it clearer. Is there anything that could break on multiple sessions?

src/test-explorer/test-manager.ts

kpodsiad · 2021-12-15T13:18:50Z

src/extension.ts

+      const refreshTests = client.onRequest(CodeLensRefreshRequest.type, () => {
+        testManager.discoverTestSuites();
+      });


Test explorer should be refreshed just like code lense. This change is possible due to scalameta/metals#3355

kpodsiad · 2021-12-15T13:22:17Z

src/test-explorer/test-manager.ts

+      }
+    };
+
+    const callback = () => (this.isRunning = false);


I experimented a little bit and it seems like this + vscode built-in cancelation mechanism seems to work (more or less). When you are in the middle of test run and you'll click to run another test site this.isRunning will prevent you from running the next suite. However, the first request is cancelled and is marked as a skipped/cancelled by vscode even though test-analyzer sets particular test items status. I think it's good enough for start and if this will be a problem we can spend more time on this mechanic. Ok @tgodzik ?

The config option was added which allows a user to switch tests UI. User can pick between Code Lenses (old way) and Test Explorer (new one). Changes are applied immediately and don't require restarting Metals server. `Test Explorer` enables to both run and debug test suites via the gutter icons (on the left of file) and via the testing panel.

kpodsiad · 2021-12-16T10:59:01Z

I squashed commits to a single one with a little bit more description.
@tgodzik @gabro have you got any comments left? I would like to put the "waiting for a metals release" label on this PR as fast as possible :D

gabro

LGTM!

tgodzik

This will be awesome!

kpodsiad mentioned this pull request Nov 9, 2021

Add test discovery endpoint scalameta/metals#3277

Merged

3 tasks

olafurpg reviewed Nov 10, 2021

View reviewed changes

src/test-provider/output-analyzer.ts Outdated Show resolved Hide resolved

kpodsiad force-pushed the test-api branch 3 times, most recently from 027152b to c4f3d59 Compare November 16, 2021 07:41

kpodsiad mentioned this pull request Nov 16, 2021

Send event with information about tests to the client scalacenter/scala-debug-adapter#157

Merged

kpodsiad force-pushed the test-api branch from d8e5ec6 to 6579cd8 Compare November 24, 2021 15:20

kpodsiad force-pushed the test-api branch from 6579cd8 to ed7bcf0 Compare December 2, 2021 18:48

kpodsiad marked this pull request as ready for review December 2, 2021 18:54

kpodsiad requested review from tgodzik and gabro December 3, 2021 15:35

kpodsiad commented Dec 3, 2021

View reviewed changes

src/test-provider/analyze-test-run.ts Outdated Show resolved Hide resolved

kpodsiad mentioned this pull request Dec 6, 2021

Add testExplorerProvider option scalameta/metals-languageclient#379

Merged

kpodsiad force-pushed the test-api branch 2 times, most recently from f023821 to b49bf73 Compare December 10, 2021 15:49

tgodzik reviewed Dec 10, 2021

View reviewed changes

src/test-provider/util.ts Outdated Show resolved Hide resolved

package.json Outdated Show resolved Hide resolved

src/test-provider/analyze-test-run.ts Outdated Show resolved Hide resolved

src/test-provider/types.ts Outdated Show resolved Hide resolved

kpodsiad force-pushed the test-api branch from 1303c30 to 2c64d1e Compare December 10, 2021 21:11

kpodsiad commented Dec 10, 2021

View reviewed changes

gabro reviewed Dec 13, 2021

View reviewed changes

tgodzik reviewed Dec 14, 2021

View reviewed changes

kpodsiad force-pushed the test-api branch from 5e2b55c to 06119d7 Compare December 15, 2021 13:16

kpodsiad commented Dec 15, 2021

View reviewed changes

kpodsiad requested review from tgodzik, gabro and olafurpg December 15, 2021 13:22

kpodsiad force-pushed the test-api branch from 06119d7 to 56ee31f Compare December 16, 2021 10:55

gabro approved these changes Dec 16, 2021

View reviewed changes

tgodzik approved these changes Dec 21, 2021

View reviewed changes

tgodzik added the wait for Metals release label Dec 21, 2021

tgodzik merged commit b7c86a6 into scalameta:main Dec 28, 2021

kpodsiad deleted the test-api branch January 21, 2022 07:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement basics of test API #755

Implement basics of test API #755

kpodsiad commented Nov 9, 2021 •

edited

Loading

tgodzik left a comment

tgodzik commented Dec 10, 2021

gabro commented Dec 10, 2021

tgodzik commented Dec 10, 2021

kpodsiad Dec 10, 2021 •

edited

Loading

kpodsiad Dec 13, 2021

gabro Dec 15, 2021

kpodsiad Dec 15, 2021 •

edited

Loading

gabro Dec 15, 2021 •

edited

Loading

gabro left a comment

gabro Dec 13, 2021

kpodsiad Dec 13, 2021

tgodzik left a comment

tgodzik Dec 14, 2021

kpodsiad Dec 14, 2021

tgodzik Dec 21, 2021

kpodsiad Dec 15, 2021

kpodsiad Dec 15, 2021

kpodsiad commented Dec 16, 2021

gabro left a comment

tgodzik left a comment

Implement basics of test API #755

Implement basics of test API #755

Conversation

kpodsiad commented Nov 9, 2021 • edited Loading

tgodzik left a comment

Choose a reason for hiding this comment

tgodzik commented Dec 10, 2021

gabro commented Dec 10, 2021

tgodzik commented Dec 10, 2021

kpodsiad Dec 10, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kpodsiad Dec 15, 2021 • edited Loading

Choose a reason for hiding this comment

gabro Dec 15, 2021 • edited Loading

Choose a reason for hiding this comment

gabro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tgodzik 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

kpodsiad commented Dec 16, 2021

gabro left a comment

Choose a reason for hiding this comment

tgodzik left a comment

Choose a reason for hiding this comment

kpodsiad commented Nov 9, 2021 •

edited

Loading

kpodsiad Dec 10, 2021 •

edited

Loading

kpodsiad Dec 15, 2021 •

edited

Loading

gabro Dec 15, 2021 •

edited

Loading