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.

Sign up for GitHub

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

Fix error not thrown #678

Merged
merged 3 commits into from
Apr 18, 2024
Merged

Fix error not thrown #678

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2f5cd3e
fix(647): fix error not thrown
lojzatran Apr 13, 2024
73f5d3a
Merge branch 'master' into 647_ts-client_not_throwing_error
lojzatran Apr 15, 2024
a90c14b
Merge branch 'master' into 647_ts-client_not_throwing_error
lojzatran Apr 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 17 additions & 0 deletions packages/platform-sdk/test/integration-tests/sdk-v3/error-cases.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
import { apiRootV3 } from '../test-utils'

describe('testing error cases', () => {
it('should throw error when a product type is not found', async () => {
try {
await apiRootV3
.productTypes()
.withId({ ID: 'non-existing-id' })
.get()
.execute()

throw new Error('Should have thrown an error')
Copy link
Contributor

@ajimae ajimae Apr 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

} catch (e) {
expect(e.statusCode).toEqual(404)
}
})
})
8 changes: 7 additions & 1 deletion packages/sdk-client-v3/src/client/client.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -176,7 +176,13 @@ export default function createClient(middlewares: ClientOptions): Client {
resolve,
...request,
})
.then(resolve)
.then((res) => {
if (res.error) {
reject(res.error)
} else {
resolve(res)
}
})
.catch(reject)
})
},
Expand Down
Loading