Fix error not thrown

[lojzatran]

Fixes: #647

@ajimae : I tried to fail the test in case the error is not thrown, but fail() is not supported by jest: jestjs/jest#11698. Do you know how to do this?

[changeset-bot]

⚠️ No Changeset found

Latest commit: a90c14b

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.83%. Comparing base (8d2df4d) to head (a90c14b).

Additional details and impacted files

@@           Coverage Diff           @@
##           master     #678   +/-   ##
=======================================
  Coverage   92.83%   92.83%           
=======================================
  Files          25       25           
  Lines         279      279           
  Branches       12       12           
=======================================
  Hits          259      259           
  Misses         20       20           

Flag	Coverage Δ
integrationtests	92.83% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[ajimae]

Fixes: #647

@ajimae : I tried to fail the test in case the error is not thrown, but fail() is not supported by jest: jestjs/jest#11698. Do you know how to do this?

So I tried it locally, the error is not being throw, it is being returned as a response as well, instead of being rejected and passed to the the .catch() block.

[lojzatran]

Fixes: #647
@ajimae : I tried to fail the test in case the error is not thrown, but fail() is not supported by jest: jestjs/jest#11698. Do you know how to do this?

So I tried it locally, the error is not being throw, it is being returned as a response as well, instead of being rejected and passed to the the .catch() block.

Can you provide more details which error is that so I can write another test for it? I tested for 404 HTTP error and it is now throwing.

[ajimae]

Indeed it does work for me now, however I think this line shouldn't be here, the sdk call chain should be the one throwing the error when the .execute() is called, then we can process the error and make our checks in the .catch() block.

So aparently, try-catch will not work here because of the async nature of the builder chain, you might want to remove the try-catch and handle the error in the .cache().

  it('should throw error when a product type is not found', async () => {
    await apiRootV3
      .productTypes()
      .withId({ ID: 'non-existing-id' })
      .get()
      .execute()
      .catch(e => {
        expect(e.statusCode).toEqual(404)
      })
  })

I tried the above and it worked just fine. So we can restructure the test a bit to include the catch block instead of throwing a generic error in the try-catch setup.

[lojzatran]

Thanks for the suggestion. I checked it and there is one problem in your test: if the error is not thrown (the code before my changes), then the test would also pass. This means that it does not test my fix.

Here I explained it a bit better:

it('should throw error when a product type is not found', async () => {
    await apiRootV3
      .productTypes()
      .withId({ ID: 'non-existing-id' })
      .get()
      .execute()
      .then(() => {
          console.log('it should throw an error when ID is non-existing and not reach this console.log')
          fail()  // <--- a method to fail the test
      })
      .catch(e => {
        expect(e.statusCode).toEqual(404)
      })
  })

There is usually fail() method to fail the test if it reaches a place it shouldn't reach, but it seems to be missing in jest. If you didn‘t encounter it before, it‘s ok and I will leave the test as it is.

[ajimae]

Since this is an integrated test, I want to believe that the test must fail, since the ID: 'non-existing-id' is actually a non existent ID.

[ajimae]

@lojzatran this 👆🏽 is my finding. What do you think?

[ajimae]

⚠️ No Changeset found

Latest commit: a90c14b

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets
Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

We didn't add a changeset to release this fix. I will go ahead and do that now.