hyperledger-cacti/cacti

test: refactor try-catch based negative scenarios with Jest matchers

Opened this issue · 2 comments

⚠️⚠️⚠️ IMPORTANT: This is an epic that serves as a template for smaller scoped issues where you perform the refactoring work explained here in a few places of the code only (instead of going all out and re-doing the entire codebase all at once).
The reason for the above is that pull requests get exponentially harder to review as their size grows, so, for these cross-cutting project-wide refactors we must break the work down to smaller chunks to allow the maintainers to keep their sanity while reviewing the code.
So, if you'd like to work on this, just open a separate issue and copy paste the content of this one, then specify a few test cases that you'll be refactoring. Then make sure to identify said test cases and their containing package in the issue title, the commit message and the PR title as well.

Description

Jest has very handy assertions (jest-extended to be more specific) which make it possible to assert the contents of an error response coming from the API.

These assertion APIs have easier to read reporting and are less confusing then the manually triggered test failures we are forced to do in the try-catch block's try leg if the operation unexpectedly succeeds with the negative scenario.

Instead of relying on clunky try-catch blocks and exception type casting we should use the native support Jest has.

As an example this is how it should be refactored, see the before and after. The two test assertions are equivalent but one is more concise, reads more like English prose and points out exactly what was not matching the assertions if there is a failure.

before

try {
  await apiClient.getKeychainEntryV1({ key });
  t.fail(
    "Failing because getKeychainEntryV1 did not throw when called with non-existent key.",
  );
} catch (ex) {
  t.ok(ex, "res7 -> ex truthy");
  const res8 = ex.response;
  t.equal(res8.status, 404, "res7.status === 404 OK");
  t.ok(res8.data, "res7.data truthy OK");
  t.ok(res8.data.error, "res7.data.error truthy OK");
  t.equal(typeof res8.data.error, "string", "res7.data.error truthy OK");
  t.true(
    res8.data.error.includes(`${key} secret not found`),
    "res7.data.error contains legible error message about missing key OK",
  );
}

after

const deletionOfNonExistentKey = apiClient.getKeychainEntryV1({ key });
await expect(deletionOfNonExistentKey).rejects.toMatchObject({
  response: expect.objectContaining({
    status: 404,
    data: expect.objectContaining({
      error: expect.stringContaining(key),
    }),
  }),
});

Acceptance Criteria

  1. The test has already been migrated to Jest.
  2. Tests are passing consistently. If the test became flaky due to the refactor, we have to investigate.

I would like to work on this Issue.

I would like to work on this Issue.

@Deepakchowdavarapu OK, I opened an issue with the limited scope here: https://github.com/hyperledger/cacti/issues/3457
Please send a comment on that one and I can assign it to you. That way it will be a small enough change to get going and you can send more PRs with more of the same changes later on as you see fit.