Add test for behaviour around expanded state by norascheuch · Pull Request #1972 · github/vscode-codeql

norascheuch · 2023-01-16T16:33:30Z

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.

charisk

Thanks for improving our test coverage! Just some small comments.

charisk · 2023-01-17T09:14:38Z

+        expandedItems![0] as VariantAnalysisUserDefinedListExpandedDbItem;
+      expect(expandedItem.listName).toEqual(listName);
+
+      // Trigger adding an item that is not in the config


I think it would be more realistic if the test added a different item (that exists), and checked that the other one (that doesn't exist) was removed.

👍 Good idea

(Optional) I think this is slightly confusing because there are 3 different lists at play. I would do the following:

Have expanded state have only 1 item (the removedVariantAnalysisList)

Trigger adding a new item in the config (the variantAnalysisList)

Check that the expanded state no longer contains the removedVariantAnalysisList

charisk · 2023-01-17T09:29:39Z

+      });
+
+      await dbManager.addDbItemToExpandedState(dbItem);
+      const expandedItems = await app.workspaceState.get<ExpandedDbItem[]>(


Does this need the await? I was under the impression it's a synchronous operation.

I just checked and indeed this is synchronous, so we can remove the await.

Good catch! Thanks

norascheuch

Thanks @charisk , changes are pushed!

norascheuch · 2023-01-17T10:08:41Z

+        expandedItems![0] as VariantAnalysisUserDefinedListExpandedDbItem;
+      expect(expandedItem.listName).toEqual(listName);
+
+      // Trigger adding an item that is not in the config


👍 Good idea

charisk

Looks good! Just a couple of minor suggestions.

charisk · 2023-01-17T11:01:01Z

+        expandedItems![0] as VariantAnalysisUserDefinedListExpandedDbItem;
+      expect(expandedItem.listName).toEqual(listName);
+
+      // Trigger adding an item that is not in the config


(Optional) I think this is slightly confusing because there are 3 different lists at play. I would do the following:

Have expanded state have only 1 item (the removedVariantAnalysisList)

Trigger adding a new item in the config (the variantAnalysisList)

Check that the expanded state no longer contains the removedVariantAnalysisList

charisk · 2023-01-17T11:02:47Z

+      });
+
+      await dbManager.addDbItemToExpandedState(dbItem);
+      const expandedItems = await app.workspaceState.get<ExpandedDbItem[]>(


I just checked and indeed this is synchronous, so we can remove the await.

norascheuch requested a review from a team as a code owner January 16, 2023 16:33

charisk reviewed Jan 17, 2023

View reviewed changes

norascheuch added 4 commits January 17, 2023 10:14

Add tests for adding and removing item to expanded state

0bccdbb

Add test for renaming expanded items

c6c7b71

Add test for expanded state cleanup

40f0839

Remove unneeded expectations and adjust test

07868d9

norascheuch force-pushed the nora/expanded-tests branch from 86c1b12 to 07868d9 Compare January 17, 2023 10:14

norascheuch commented Jan 17, 2023

View reviewed changes

charisk approved these changes Jan 17, 2023

View reviewed changes

Merge comments

e6c6e8a

norascheuch enabled auto-merge January 17, 2023 12:30

norascheuch merged commit c5722d7 into main Jan 17, 2023

norascheuch deleted the nora/expanded-tests branch January 17, 2023 13:23

Conversation

norascheuch commented Jan 16, 2023

Checklist

Uh oh!

charisk left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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!

norascheuch left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants