Add tests for Model Details View by norascheuch · Pull Request #2735 · github/vscode-codeql

norascheuch · 2023-08-22T13:13:26Z

Adds tests for

model details panel set state
reveal logic
model details provider getChildren

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.
[Maintainers only] If this pull request makes user-facing changes that require documentation changes, open a corresponding docs pull request in the github/codeql repo and add the ready-for-doc-review label there.

robertbrignull

I like what the tests are testing. That's exactly what I meant and wanted to see! 🙇🏼

I think the tests would be better and more robust to changes if we reduced the frequency of mockedObject and defined objects more fully. What are other people's thoughts?

robertbrignull · 2023-08-22T14:53:33Z

+    });
+
+    it("should return [] if item is a usage", async () => {
+      const item = { usages: undefined } as unknown as ModelDetailsTreeViewItem;


Can we avoid using as unknown as X? Instead define a proper Usage object.

robertbrignull · 2023-08-22T15:00:33Z

+
+  describe("getChildren", () => {
+    const externalApiUsages: ExternalApiUsage[] = [
+      mockedObject<ExternalApiUsage>({ supported: true }),


I see that the desire here is to avoid the pain of providing full objects, but mockedObject is going to potentially cause some issues here that will be hard to debug for future developers. I think you've already hit this, as evidenced by needing to pass usages: undefined in some cases.

In ModelDetailsDataProvider we use isExternalApiUsage to determine if the object we have is an ExternalApiUsage or a Usage. Unfortunately this check won't work correctly when given an object created using mockedObject, or at least you have to know exactly what you're doing and pass the right field values to make isExternalApiUsage behave correctly. While it is possible to have everything work correctly like this, it's quite error prone.

While it's is great for things like CodeQLCliServer, I would recommend not using mockedObject for types like ExternalApiUsage and Usage. How much effort would it be to not do this?

robertbrignull · 2023-08-22T15:01:59Z

+        methodParameters: "test",
+      },
+    ];
+    const dbItem = mockedObject<DatabaseItem>({


We define the exact same dbItem in each describe block. Could we move the definition one level higher, like where we make mockCliServer?

robertbrignull · 2023-08-22T15:04:58Z

+    });
+
+    it("should reveal the correct item in the tree view", async () => {
+      const externalApiUsages = mockedObject<ExternalApiUsage[]>([


Again as commented on the other file, I suggest avoiding using mockedObject for the ExternalApiUsage type. We already define a full ExternalApiUsage object earlier in this file, so could we reuse that? Or perhaps add a factory method for making ExternalApiUsage objects (see the test/factories directory)?

charisk

LGTM! Just some comments for potential improvements.

charisk · 2023-08-24T07:21:20Z

+    });
+
+    it("should show all externalApiUsages if hideModeledApis is false and item is undefined", async () => {
+      const hideModeledApis: boolean = false;


(Minor) you shouldn't need to define the type

Suggested change

const hideModeledApis: boolean = false;

const hideModeledApis = false;

Same on L132 and a few places in model-details-panel.test.ts - worth doing a search for const hideModeledApis: boolean = to find all occurrences.

charisk · 2023-08-24T07:26:55Z

+    beforeEach(() => {
+      mockTreeView = {
+        reveal: jest.fn(),
+      } as unknown as TreeView<unknown>;


This is similar to as any as X so we should ideally avoid it. Is there a way to get around it easily, or is creating mock TreeView<>T> items too painful? I'm happy to take it on as tech debt too.

The problem lies in the reveal: jest.fn(). Without it we could remove the as unknown. With it we would have to mock the vs code reveal method better. I'm not sure if that is easily possible? We only check if reveal was called, not what it does. Therefore I leaned towards leaving it as is. However, can we introduce other errors by using as unknown here apart from the reveal method?

Lets leave it as is. I imagine creating the mock properly isn't trivial.

Thanks for talking this through. I think mockedObject could be a good solution for this case. I've opened #2768 for this where I've tried to explain my reasoning, so let me know what you think.

charisk · 2023-08-24T07:30:11Z

+      expect(dataProvider.getChildren(externalApiUsage)).toEqual([usage]);
+    });
+
+    it("should show all externalApiUsages if hideModeledApis is false and item is undefined", async () => {


(Minor) The item is undefined bit confused me a little bit but I realised what it meant once I got to know the code a bit better. We don't have to change it, but do you think the following description is accurate/better:

Suggested change

it("should show all externalApiUsages if hideModeledApis is false and item is undefined", async () => {

it("should show all externalApiUsages if hideModeledApis is false and looking at the root", async () => {

sounds good, will rename!

norascheuch added 4 commits August 22, 2023 08:55

Add mode details panel test

cb2802e

Move dataProvider creation to beforeEach

6e4e89a

Add getChildren tests

392e76d

Add reveal test

45f7d22

norascheuch added the secexp label Aug 22, 2023

norascheuch requested a review from a team as a code owner August 22, 2023 13:13

robertbrignull reviewed Aug 22, 2023

View reviewed changes

create and use factories

c56936e

norascheuch requested a review from a team as a code owner August 23, 2023 16:06

charisk approved these changes Aug 24, 2023

View reviewed changes

rename test and remove boolean type

0daa06b

norascheuch merged commit 422f6de into main Aug 24, 2023

norascheuch deleted the nora/test-model-details-view branch August 24, 2023 10:33

	const hideModeledApis: boolean = false;
	const hideModeledApis = false;

	it("should show all externalApiUsages if hideModeledApis is false and item is undefined", async () => {
	it("should show all externalApiUsages if hideModeledApis is false and looking at the root", async () => {

Conversation

norascheuch commented Aug 22, 2023

Checklist

Uh oh!

robertbrignull left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

charisk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

robertbrignull left a comment •

edited

Loading