Add tests for removing remote queries and variant analyses

[koesie10]

Unfortunately, one of the tests we have for local queries doesn't seem to be working for variant analyses. I'm not sure why it isn't working, but I think it's better to get the rest of the integration tests in and then figure out what's going on with that one.

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.

[elenatanasoiu]

The descriptions of some of the tests are almost identical but they're testing different types of items.

For the sake of making these more readable if one test fails, could we wrap these deletion tests in describe blocks that state the premise of the test?

Like so:

describe("when the item being removed is selected", async () => {
  describe("when the item is a remote query", async () => {
    it("should remove it and select a new one", async () => {
       ...
    });
  });

  describe("when the item is a variant analysis", async () => {
    it("should remove it and select a new one", async () => {
      ...
    })
});

describe("when the item being removed is not selected", async () => {
  describe("when the item is a remote query", async () => {
    it("should remove it and not change the selection", async () => {
       ...
    });
  });

  describe("when the item is a variant analysis", async () => {
    it("should remove it and not select a new one", async () => {
      ...
    })
  })
});

Totally optional side quest since the issue was already present here: when you have an and in the test description it's a good indicator that the test is doing too much and should probably be two tests, each checking a single assumption.

If we wanted to split up these tests, we'd need to move the setup for removing an item in a before block (as it's quite long and we want to be as DRY as possible).

So we'd end up with:

before(() => {
    // Item removal goes here and gets executed once
});
  
it("should remove a remote query item", async () => {
  // expect the item to be removed
});

it("should not change the selected item", async () => {
  // expect the selection to stay the same
});

When the test fails, you'll be able to pinpoint the piece of behaviour that's broken more easily. It's more of a good muscle habit to develop and a favour to future you rather than a massive speed benefit for these particular tests.

[koesie10]

Thanks for the suggestion, it totally makes sense. I've now split them up like you suggested, although I've changed the order (from selected -> type to type -> selected) since that's how they were also currently structured. This should make it much easier to reason about these tests and I've been able to remove the commented out part and replace it by an it.skip.

[elenatanasoiu]

Looks good as is, but have left a suggestion. Thanks for writing tests ❤️