Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
petermetz opened this issue Aug 3, 2024 · 2 comments
Open
Labels
good-first-issue Good for newcomers good-first-issue-100-introductory good-first-issue-200-intermediate Hacktoberfest Hacktoberfest participants are welcome to take a stab at issues marked with this label. P4 Priority 4: Low Tests Anything related to tests be that automatic or manual, integration or unit, etc.

Comments

@petermetz
Copy link
Contributor

petermetz commented Aug 3, 2024

⚠️⚠️⚠️ 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.
@petermetz petermetz added good-first-issue Good for newcomers Hacktoberfest Hacktoberfest participants are welcome to take a stab at issues marked with this label. good-first-issue-100-introductory good-first-issue-200-intermediate Tests Anything related to tests be that automatic or manual, integration or unit, etc. P4 Priority 4: Low labels Aug 3, 2024
petermetz referenced this issue in petermetz/cacti Aug 3, 2024
1. The Jest test execution was turned off for the AWS SM keychain plugin
for some reason so this PR enables it back and while at it
2. I also refactored one of the test cases to have less linter warnings
about us casting to `any`.
3. And on top of that I also migrated one of the test cases to Jest from
TAP/tape so that we are making some progress with the test coverage at
the same time.

One more notable thing is that I refactored the negative test assertions
while migrating to Jest so the test case now uses Jest native assertions
to check what exactly the error message returned by the API call for
deleting a non-existent item looks like. This is an example of the
refactoring that this issue is about:
https://github.com/hyperledger/cacti/issues/3455

Signed-off-by: Peter Somogyvari <peter.somogyvari@accenture.com>
@Deepakchowdavarapu
Copy link

I would like to work on this Issue.

@petermetz
Copy link
Contributor Author

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.

petermetz referenced this issue in petermetz/cacti Aug 8, 2024
1. The Jest test execution was turned off for the AWS SM keychain plugin
for some reason so this PR enables it back and while at it
2. I also refactored one of the test cases to have less linter warnings
about us casting to `any`.
3. And on top of that I also migrated one of the test cases to Jest from
TAP/tape so that we are making some progress with the test coverage at
the same time.

One more notable thing is that I refactored the negative test assertions
while migrating to Jest so the test case now uses Jest native assertions
to check what exactly the error message returned by the API call for
deleting a non-existent item looks like. This is an example of the
refactoring that this issue is about:
https://github.com/hyperledger/cacti/issues/3455

Signed-off-by: Peter Somogyvari <peter.somogyvari@accenture.com>
petermetz referenced this issue Aug 9, 2024
1. The Jest test execution was turned off for the AWS SM keychain plugin
for some reason so this PR enables it back and while at it
2. I also refactored one of the test cases to have less linter warnings
about us casting to `any`.
3. And on top of that I also migrated one of the test cases to Jest from
TAP/tape so that we are making some progress with the test coverage at
the same time.

One more notable thing is that I refactored the negative test assertions
while migrating to Jest so the test case now uses Jest native assertions
to check what exactly the error message returned by the API call for
deleting a non-existent item looks like. This is an example of the
refactoring that this issue is about:
https://github.com/hyperledger/cacti/issues/3455

Signed-off-by: Peter Somogyvari <peter.somogyvari@accenture.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good-first-issue Good for newcomers good-first-issue-100-introductory good-first-issue-200-intermediate Hacktoberfest Hacktoberfest participants are welcome to take a stab at issues marked with this label. P4 Priority 4: Low Tests Anything related to tests be that automatic or manual, integration or unit, etc.
Projects
None yet
Development

No branches or pull requests

2 participants