Display test results in CodeLens #24

samcragg · 2017-11-13T22:16:59Z

Hope you don't mind me suggesting an enhancement. Also, it's my first foray into TypeScript so happy to change things to match your preferences. I wanted to display the test result status next to the method, like VS does so I looked at getting the results from the most recent test run.:

The results are outputted to a TRX file and then that file is parsed to get the test results. Finally, the vscode.executeDocumentSymbolProvider command is used to get all the symbols in the current document to match the method to the test name.

Things to note. I've tested it with xUnit - it probably needs checking with NUnit as well. Also, I'm using emoji's to display the results, as you can only have text in the CodeLens. This looks fine on Windows 10 but might need a switch to use normal text instead of emoji's in case it looks wrong on other OS's.

Finally, there are things I'd like to enhance but not sure how to implement. I'd like it so you can click on the status and a popup show you the test output (useful for failing tests).

+ Output the test results to the TRX logger with a temporary filename + Listen to changes to the temporary directory to see when the TRX file has been written to.

+ Add dependency on xmldom + Parse the XML into strongly types objects

formulahendry

Thanks for your PR! Please help resolve the TSLint error in CI first. 😄

samcragg · 2017-11-14T07:22:34Z

Sorry about that, still learning the ropes 😄

formulahendry · 2017-11-15T06:35:03Z

src/testResultsFile.ts

+ const nodes = xdoc.documentElement.getElementsByTagName("UnitTestResult");
+
+ // TSLint wants to use for-of here, but nodes doesn't support it
+ for (let i = 0; i < nodes.length; i++) { // tslint:disable-line


You could use for (const node of nodes): https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/for...of

I did try that, however, at runtime it throws an exception TypeError: undefined is not a function as I don't believe HTMLCollection (returned by getElementsByTagName) has the iterator property so it can't be used in for-of. May be there's a better XML library I could use to parse the TRX file?

In that case, let's just keep this for loop.

formulahendry · 2017-11-15T06:42:15Z

src/testResultsFile.ts

+ let timeout: NodeJS.Timer;
+ this.watcher = fs.watch(tempFolder, (eventType, fileName) => {
+ if ((fileName === TestResultsFile.ResultsFileName) && (eventType === "change")) {
+ if (!timeout) {


I think you could use clearTimeout here

I'm not sure that does the same thing, as it a timeout previously established by calling setTimeout, however, we're calling it in the callback so there's nothing to cancel. I'm setting it to null so that I can see if I've already triggered a wait or not. I can introduce another flag variable if that would make the code easier to follow?

I think you could refer to https://github.com/formulahendry/vscode-docker-explorer/blob/master/src/dockerTreeBase.ts#L21:31 The logic should be similar.
In current implementation, if the change event lasts for 30 seconds continuously (say every 100ms), parseResults will be called 30 times.
With the clearTimeout, it would be called only once (if the change event is 100ms, or less then 1000ms).

When the test is running, I guess the trx file will be changed continuously. I am not sure whether the TRX will always be a valid XML and could be read normally during the test is running. If it is, I think current implementation is good, since we could have a quick update when test is running.

Like it, done! 😄

formulahendry · 2017-11-15T07:29:52Z

src/testResultsFile.ts

+ try {
+ this.watcher.close();
+ fs.unlinkSync(this.resultsFile);
+ } catch {


miss (error) --> catch (error)

+ Use clearTimeout when filtering multiple file change events + Fix empty catch block

formulahendry · 2017-11-15T08:03:25Z

src/dotnetTestExplorer.ts

+ * Gets the dotnet test argument to speicfy the output for the test results.
+ */
+ private outputTestResults(): string {
+ return " --logger 'trx;LogFileName=" + this.resultsFile.fileName + "'";


I test it on Windows, it is failed, After I change single quote to double quote, it is working. How about you?

It did work for me, but I've replaced with double quotes and it still works for me so hopefully that should fix it for you

I guess you are using PowerShell. Both are working on PowerShell, while single quote does not work on CMD.

formulahendry · 2017-11-15T08:19:30Z

src/testStatusCodeLensProvider.ts

+ for (const symbol of symbols.filter((x) => x.kind === SymbolKind.Method)) {
+ const fullName = symbol.containerName + "." + symbol.name;
+ for (const result of results.values()) {
+ if (result.testName.endsWith(fullName)) {


There is a corner case (when only namespace is different):

The symbol provider doesn't return anything for the namespace and the container of the class is empty, so would need to look at perhaps using omni-sharp directly? There's actually an issue with the test discovery with NUnit that only lists the method names, so if two tests exist with the same method name in different classes, NUnit only returns one of them.

Not sure how you want to approach this one? Accept it as a corner case until the extension uses omni-sharp to parse the files to discover the unit test methods?

I think this corner case could be accepted.

If you have interest to look at omni-sharp or other approach, it is warmly welcome! You could send PR in the future.

formulahendry · 2017-11-15T08:34:40Z

src/dotnetTestExplorer.ts

@@ -198,7 +215,7 @@ export class DotnetTestExplorer implements TreeDataProvider<TestNode> {
 return new Promise((c, e) => {
 try {
 const results = Executor
- .execSync(`dotnet test -t -v=q${this.checkBuildOption()}${this.checkRestoreOption()}`, this.testDirectoryPath)
+ .execSync(`dotnet test -t -v=q${this.getDotNetTestOptions()}`, this.testDirectoryPath)


You did not add ${this.outputTestResults()}, so the test result will not be shown when VS Code starts. Is it by design?

Oh, please ignore. It only lists the tests.

formulahendry · 2017-11-15T08:35:29Z

src/testResultsFile.ts

+}
+
+export class TestResultsFile implements Disposable {
+ private static readonly ResultsFileName = "TestExplorerResults.trx";


What if we have multiple test project opened in different VS Code?

I'm now using a temporary random folder to store the results, so now works across multiple instances

formulahendry · 2017-11-15T08:57:19Z

src/testStatusCodeLensProvider.ts

+ return this.onDidChangeCodeLensesEmitter.event;
+ }
+
+ public provideCodeLenses(document: TextDocument, token: CancellationToken): CodeLens[] | Thenable<CodeLens[]> {


Try with xUnit, NUnit and MSTest. Seems only xUnit is showing the test result icon. It would be great if you could also support NUnit and MSTest. 😃

This should now be fixed in the latest version, I've tried xUnit, MSTest and NUnit (the later two have the same format)

formulahendry · 2017-11-15T09:07:13Z

src/testStatusCodeLensProvider.ts

+ }
+
+ private checkEnabledOption(): void {
+ this.enabled = workspace.getConfiguration("csharp").get<boolean>("testsCodeLens.enabled", true);


Not sure it is better to add an extra boolean setting, in case user want to only show the run test | debug test. When user click run test, our test result icon would not updated. This may confuse users.

Added an extra setting dotnet-test-explorer.showCodeLens

+ Map the test definition to the test result in the TRX file

+ Use double quotes instead of single quotes

+ Use a specific setting for this extension to determine whether to show/hide the CodeLens test status

+ Use a random temporary folder to store the test results

formulahendry · 2017-11-17T08:27:47Z

src/testResultsFile.ts

+ // delay before we read anything to avoid doing too much work
+ const me = this;
+ let changeDelay: NodeJS.Timer;
+ this.watcher = fs.watch(tempFolder, (eventType, fileName) => {


Only do this when 'showCodeLens' is true

Good catch, done

formulahendry · 2017-11-17T08:28:45Z

src/dotnetTestExplorer.ts

+ * Gets the dotnet test argument to speicfy the output for the test results.
+ */
+ private outputTestResults(): string {
+ return " --logger \"trx;LogFileName=" + this.resultsFile.fileName + "\"";


return empty string when 'showCodeLens' is false

formulahendry · 2017-11-17T08:34:00Z

src/testResultsFile.ts

+ private watcher: fs.FSWatcher;
+
+ public constructor() {
+ const tempFolder = fs.mkdtempSync(path.join(os.tmpdir(), "test-explorer-"));


Same here. Only do this when 'showCodeLens' is true.
Just want to make sure no unnecessary code is running

formulahendry · 2017-11-17T08:34:47Z

src/testResultsFile.ts

+
+ public dispose(): void {
+ try {
+ this.watcher.close();


Also handle when when 'showCodeLens' is false

+ Only create the temproary folder if the CodeLens setting is enabled + Don't output the test results if CodeLens is disabled

formulahendry · 2017-11-23T14:03:40Z

src/testResultsFile.ts

+ const me = this;
+ let changeDelay: NodeJS.Timer;
+ this.watcher = fs.watch(folder, (eventType, fileName) => {
+ if ((fileName === TestResultsFile.ResultsFileName) && (eventType === "change")) {


Test it on macOS, when first run the test, it will only have eventType === "rename". So we could just remove (eventType === "change")

formulahendry · 2017-11-23T14:09:19Z

src/testStatusCodeLens.ts

+export class TestStatusCodeLens extends CodeLens {
+ public static parseOutcome(outcome: string): string {
+ if (outcome === "Passed") {
+ return "✔️";


Test on macOS, it is showing a black check mark (https://emojipedia.org/heavy-check-mark/) which is hard to see.
Two improvements:

Using another better check mark: https://emojipedia.org/white-heavy-check-mark/. Or you may have a better one? 😄

Using the unicode, it would be good to maintain

I've added settings now so the user can override what we display.

I've kept the heavy check mark for platforms apart from OS X, in which case it uses the white heavy check mark (I'm not a big fan of it, but I can't find any alternatives).

Since the user can specify the text, they could even switch to "passing", "FAILING" etc if they want so hopefully this is OK?

Looks great to me!

formulahendry · 2017-11-23T14:09:58Z

src/testStatusCodeLens.ts

+ if (outcome === "Passed") {
+ return "✔️";
+ } else if (outcome === "Failed") {
+ return "❌";


Same here, using unicode: https://emojipedia.org/cross-mark/

formulahendry · 2017-11-23T14:10:02Z

src/testStatusCodeLens.ts

+ } else if (outcome === "Failed") {
+ return "❌";
+ } else if (outcome === "NotExecuted") {
+ return "️️⚠️";


Same here, using unicode

formulahendry

I left some comments which should just have some small code change. I think we are closing to ship it!

+ Moved fields after methods

formulahendry · 2017-11-25T07:28:33Z

src/utility.ts

+ const osx = platform() === "darwin";
+
+ Utility.showCodeLens = configuration.get<boolean>("showCodeLens", true);
+ Utility.failed = Utility.getLensText(configuration, "codeLensPassed", "\u274c"); // Cross Mark


codeLensPassed?

Good catch, I've actually tested them all this time before submitting (and had my ☕️!)

formulahendry · 2017-11-25T07:28:43Z

src/utility.ts

+ Utility.showCodeLens = configuration.get<boolean>("showCodeLens", true);
+ Utility.failed = Utility.getLensText(configuration, "codeLensPassed", "\u274c"); // Cross Mark
+ Utility.passed = Utility.getLensText(configuration, "codeLensPassed", osx ? "\u2705" : "\u2714"); // White Heavy Check Mark / Heavy Check Mark
+ Utility.skipped = Utility.getLensText(configuration, "codeLensPassed", "\u26a0"); // Warning


codeLensPassed?

Don't commit code before coffee...

formulahendry · 2017-11-25T17:07:58Z

@samcragg , it is all good for C# in xUnit, NUnit and MSTest, also Windows and macOS. I just try it on F# but not show the CodeLens. I think F# extension also implements the executeDocumentSymbolProvider similar to C# extension. Would you please take a look? Or you want to support C# first?

formulahendry · 2017-11-26T06:17:41Z

I just realize that you registerCodeLensProvider for csharp only. I think it is good enough to support C# first. 😃 Thank you for your contribution! This feature is awesome! If you have interest, welcome PR to support F# or any other features.

formulahendry

samcragg · 2017-11-26T06:27:54Z

Thanks! I'll take a look at F# and submit another PR.

I also want to make it so you can click on the lens and it shows you the test output inline (similar to how clicking on the references CodeLens shows them inline) so you can see why a test failed without having to dig through the output yourself (the output is in the XML, just need somewhere to display it).

formulahendry · 2017-11-26T06:49:34Z

Thanks @samcragg ! That would be great!

samcragg added 3 commits November 13, 2017 07:19

Add support for monitoring when there are new test results

ab22289

+ Output the test results to the TRX logger with a temporary filename + Listen to changes to the temporary directory to see when the TRX file has been written to.

Parse TRX file

e9ca54b

+ Add dependency on xmldom + Parse the XML into strongly types objects

Add CodeLens support

019450d

formulahendry requested changes Nov 14, 2017

View reviewed changes

Fix TSLint errors

1b65c4e

formulahendry reviewed Nov 15, 2017

View reviewed changes

Code review fixes

83afd27

+ Use clearTimeout when filtering multiple file change events + Fix empty catch block

formulahendry reviewed Nov 15, 2017

View reviewed changes

samcragg added 4 commits November 16, 2017 07:06

Add support for MSTest/NUnit

f38bfdc

+ Map the test definition to the test result in the TRX file

Fix passing parameters to dotnet test

d741e97

+ Use double quotes instead of single quotes

Add setting for CodeLens status

a75d816

+ Use a specific setting for this extension to determine whether to show/hide the CodeLens test status

Support multiple instances

c816619

+ Use a random temporary folder to store the test results

formulahendry reviewed Nov 17, 2017

View reviewed changes

Reduce amount of work if CodeLens is not eneabled

10bef7b

+ Only create the temproary folder if the CodeLens setting is enabled + Don't output the test results if CodeLens is disabled

formulahendry reviewed Nov 23, 2017

View reviewed changes

samcragg added 2 commits November 25, 2017 07:16

Add ability to customise CodeLens display text

59e8e66

TSLint alterations

5c4eda9

+ Moved fields after methods

formulahendry reviewed Nov 25, 2017

View reviewed changes

Check the correct CodeLens settings

89baa35

Don't commit code before coffee...

formulahendry approved these changes Nov 26, 2017

View reviewed changes

formulahendry merged commit 2aceb7f into formulahendry:master Nov 26, 2017

Display test results in CodeLens #24

Display test results in CodeLens #24

Conversation

samcragg commented Nov 13, 2017

formulahendry left a comment

Choose a reason for hiding this comment

samcragg commented Nov 14, 2017

Choose a reason for hiding this comment

samcragg Nov 15, 2017 • edited Loading

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

formulahendry Nov 15, 2017 • edited Loading

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

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

formulahendry 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

formulahendry commented Nov 25, 2017

formulahendry commented Nov 26, 2017

formulahendry left a comment

Choose a reason for hiding this comment

samcragg commented Nov 26, 2017

formulahendry commented Nov 26, 2017

samcragg Nov 15, 2017 •

edited

Loading

formulahendry Nov 15, 2017 •

edited

Loading