Restructure the tree in the Test Explorer View #624

aeisenberg · 2020-10-19T22:06:18Z

With this change we display the tree based on the file system not based
on ql-packs. We also merge test folders whose only child is another
test folder.

Resolves #595

Checklist

CHANGELOG.md has been updated to incorporate all user visible changes made by this pull request.
Issues have been created for any UI or other user-facing changes made by this pull request.
@github/docs-content-dsp has been cc'd in all issues for UI or other user-facing changes made by this pull request.

extensions/ql-vscode/src/qltest-discovery.ts

adityasharad · 2020-10-20T21:32:18Z

extensions/ql-vscode/src/qltest-discovery.ts

@@ -98,12 +110,12 @@ interface QLTestDiscoveryResults {
  /**
   * The root test directory for each QL pack that contains tests.


Update comment please :)

adityasharad · 2020-10-20T21:32:25Z

extensions/ql-vscode/src/qltest-discovery.ts

@@ -98,12 +110,12 @@ interface QLTestDiscoveryResults {
  /**
   * The root test directory for each QL pack that contains tests.
   */
-  testDirectories: QLTestDirectory[];
+  testDirectory: QLTestDirectory | undefined;
  /**
   * The list of file system paths to watch. If any of these paths changes, the discovery results


Update comment please :)

adityasharad · 2020-10-20T21:33:18Z

extensions/ql-vscode/src/qltest-discovery.ts

-  public get onDidChangeTests(): Event<void> { return this._onDidChangeTests.event; }
+  public get onDidChangeTests(): Event<void> {
+    return this._onDidChangeTests.event;
+  }

  /**
   * The root test directory for each QL pack that contains tests.


Update - I assume this is just the workspace folder, viewed as a QLTestDirectory.

adityasharad · 2020-10-20T21:42:27Z

extensions/ql-vscode/src/qltest-discovery.ts

-
-    return { testDirectories, watchPaths };
+    return {
+      testDirectory: await this.discoverTests(),


We now unconditionally run test discovery on all workspace folders. How does this scale/perform when we have a large workspace with many folders, or a single large repo (like our internal codebase)? Does it also attempt to search the source archives that we add to the workspace in the extension, and if so can we skip them?

I'll try this out on different repos to see.

aeisenberg

Oh...never submitted these comments from days ago.

extensions/ql-vscode/src/qltest-discovery.ts

aeisenberg · 2020-10-23T16:42:55Z

extensions/ql-vscode/src/qltest-discovery.ts

-
-    return { testDirectories, watchPaths };
+    return {
+      testDirectory: await this.discoverTests(),


I'll try this out on different repos to see.

adityasharad · 2020-11-05T00:20:27Z

Some outstanding doc comments, and changelog conflicts.

With this change we display the tree based on the file system not based on ql-packs. We also merge test folders whose only child is another test folder. Resolves github#595

aeisenberg · 2020-11-05T06:16:56Z

Update here. I found a bug in how discovery works. QLTest discovery runs twice at startup. The reason is this:

QLPack discovery starts
QLTest discovery starts
When QLPack discovery finishes, it triggers a new discovery for QLTest. Even though QLTest discovery just finished.

This is the case with the main branch right now. In the new setup we don't rely on QLPacks at all. It can be completely removed. And code is significantly simplified.

Now the bad news. Each initial discovery operation in the new way (from the root of the workspace folder takes between 500-1500ms. Under the current way, where we do discovery from the roots of the test qlpacks, initial discovery takes between 100-200ms.

For whatever reason, subsequent discoveries under the new way are quicker (700-800ms) and slower under the old way (400-500ms). This data was not captured very scientifically.

I personally think that this difference in time is not a deal breaker. Initial discovery being slow is less important since that happens during other initialization so, a little slowness for tests is not very noticeable. And the fact that it takes about 2x longer for subsequent discoveries is not noticeable either since this happens during a save, while editing.

With this in mind, I will refactor to remove the double discovery operations. This will mean that qlpack discovery will be removed.

We no longer rely on qlpacks for our ql test structure. For this reason, we no longer need to do qlpack discovery.

aeisenberg · 2020-11-05T22:51:12Z

Ping! @github/docs-content-dsp This PR changes the layout structure of the QL test tree. Before this PR, the test tree was scoped by workspace folders, then by ql packs in those folders.

The new layout that the ql test tree matches the filesystem. This makes it easier to map tests back to their location in the filesystem.

hubot · 2020-11-05T23:01:00Z

📚 ~~Thanks for the docs ping! 🛎️ This was added to our docs first-responder project board. A team member will be along shortly to review this for docs impact.~~

EDIT: No docs impact ✔️

shati-patel

Ping! @github/docs-content-dsp This PR changes the layout structure of the QL test tree. Before this PR, the test tree was scoped by workspace folders, then by ql packs in those folders.

The new layout that the ql test tree matches the filesystem. This makes it easier to map tests back to their location in the filesystem.

Thanks, makes sense! We don't go into detail about the structure of the Test Explorer view in the docs, so this doesn't need explicit documentation 😃

shati-patel · 2020-11-09T09:08:40Z

extensions/ql-vscode/CHANGELOG.md

@@ -28,6 +28,7 @@
 - Add a `View DIL` command on query history items. This opens a text editor containing the Datalog Intermediary Language representation of the compiled query.
 - Remove feature flag for the AST Viewer. For more information on how to use the AST Viewer, [see the documentation](https://help.semmle.com/codeql/codeql-for-vscode/procedures/exploring-the-structure-of-your-source-code.html).
 - The `codeQL.runningTests.numberOfThreads` setting is now used correctly when running tests.
+- Alter structure of the _Test Explorer_ tree. It now follows the structure of the filesystem instead of the qlpacks.


Suggested change

- Alter structure of the _Test Explorer_ tree. It now follows the structure of the filesystem instead of the qlpacks.

- Alter structure of the _Test Explorer_ tree. It now follows the structure of the filesystem instead of the QL packs.

Whoops I think this suggestion got omitted.

aeisenberg requested review from dbartol and adityasharad October 19, 2020 22:06

adityasharad reviewed Oct 20, 2020

View reviewed changes

aeisenberg added the Complexity: Medium Requires a moderate level of detail in design or review. label Oct 23, 2020

aeisenberg commented Nov 4, 2020

View reviewed changes

aeisenberg added 2 commits November 4, 2020 20:33

Update changelog

34f683b

Restructure the tree in the Test Explorer View

5cc9f83

With this change we display the tree based on the file system not based on ql-packs. We also merge test folders whose only child is another test folder. Resolves github#595

aeisenberg force-pushed the aeisenberg/restructure-test-tree branch from ea3db03 to 5cc9f83 Compare November 5, 2020 06:03

Remove QLPackDiscovery

a7a582d

We no longer rely on qlpacks for our ql test structure. For this reason, we no longer need to do qlpack discovery.

aeisenberg force-pushed the aeisenberg/restructure-test-tree branch from 0e9e2a9 to a7a582d Compare November 5, 2020 14:47

shati-patel reviewed Nov 9, 2020

View reviewed changes

adityasharad approved these changes Nov 9, 2020

View reviewed changes

aeisenberg merged commit 245496c into github:main Nov 9, 2020

aeisenberg deleted the aeisenberg/restructure-test-tree branch November 24, 2020 22:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Restructure the tree in the Test Explorer View #624

Restructure the tree in the Test Explorer View #624

aeisenberg commented Oct 19, 2020 •

edited

Loading

adityasharad Oct 20, 2020

adityasharad Oct 20, 2020

adityasharad Oct 20, 2020

adityasharad Oct 20, 2020

aeisenberg Oct 23, 2020

aeisenberg left a comment

aeisenberg Oct 23, 2020

adityasharad commented Nov 5, 2020

aeisenberg commented Nov 5, 2020

aeisenberg commented Nov 5, 2020

hubot commented Nov 5, 2020 •

edited by shati-patel

Loading

shati-patel left a comment

shati-patel Nov 9, 2020

adityasharad Nov 9, 2020

		@@ -98,12 +110,12 @@ interface QLTestDiscoveryResults {
		/**
		* The root test directory for each QL pack that contains tests.

	- Alter structure of the _Test Explorer_ tree. It now follows the structure of the filesystem instead of the qlpacks.
	- Alter structure of the _Test Explorer_ tree. It now follows the structure of the filesystem instead of the QL packs.

Restructure the tree in the Test Explorer View #624

Restructure the tree in the Test Explorer View #624

Conversation

aeisenberg commented Oct 19, 2020 • edited Loading

Checklist

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

aeisenberg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adityasharad commented Nov 5, 2020

aeisenberg commented Nov 5, 2020

aeisenberg commented Nov 5, 2020

hubot commented Nov 5, 2020 • edited by shati-patel Loading

shati-patel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aeisenberg commented Oct 19, 2020 •

edited

Loading

hubot commented Nov 5, 2020 •

edited by shati-patel

Loading