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

feat(sdk): improved error reporting by adding detail to getConfig and getDmmf error #13736

Merged
merged 34 commits into from Jun 24, 2022

Conversation

jkomyno
Copy link
Contributor

@jkomyno jkomyno commented Jun 9, 2022

Improve error reporting occurring in #13717.

Screenshot 2022-06-09 at 16 51 14

@jkomyno jkomyno changed the title sdk: improved error reporting by adding detail to getConfig and getDmmf error feat(sdk): improved error reporting by adding detail to getConfig and getDmmf error Jun 9, 2022
@jkomyno jkomyno added this to the 4.0.x milestone Jun 9, 2022
@jkomyno jkomyno marked this pull request as ready for review June 15, 2022 13:51
@jkomyno jkomyno requested a review from a team June 15, 2022 13:51
@jkomyno jkomyno requested a review from Jolg42 as a code owner June 15, 2022 13:51
@jkomyno jkomyno requested review from danstarns and removed request for a team June 15, 2022 13:51
@janpio janpio self-requested a review June 15, 2022 15:47
Copy link
Member

@janpio janpio left a comment

Choose a reason for hiding this comment

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

some snapshot updates or test differentiation based on engine type required it seems

@Jolg42
Copy link
Member

Jolg42 commented Jun 17, 2022

Looks good but I can see tests are failing in sdk with

FAIL src/__tests__/engine-commands/queryEngineCommons.test.ts
  loadNodeAPILibrary
    ✕ error path (121 ms)
    ✕ error path, openssl (122 ms)
    ✕ happy path (116 ms)

and in client with

     - Get DMMF: Schema parsing - Error while interacting with query-engine-node-api library
    + Get DMMF: Schema parsing - Error while interacting with query-engine binary

I guess it's because there is only one snapshot for library and engine and we need to do something here

@jkomyno jkomyno enabled auto-merge (squash) June 17, 2022 15:57
@jkomyno jkomyno requested a review from Jolg42 June 20, 2022 09:40
Copy link
Member

@Jolg42 Jolg42 left a comment

Choose a reason for hiding this comment

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

Nice one with the spy/mock tests!

See last comment about a test before merging 🚢

…mmons.test.ts

Co-authored-by: Joël Galeran <Jolg42@users.noreply.github.com>
@Jolg42 Jolg42 disabled auto-merge June 24, 2022 08:21
@Jolg42 Jolg42 merged commit 8e90f8a into main Jun 24, 2022
@Jolg42 Jolg42 deleted the feat/detail-errors-on-panics branch June 24, 2022 08:21
@janpio
Copy link
Member

janpio commented Jun 24, 2022

Is there an issue this can close so this can appear in the issue list in our release notes?

@jkomyno
Copy link
Contributor Author

jkomyno commented Jun 24, 2022

Is there an issue this can close so this can appear in the issue list in our release notes?

This PR closes #14006

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants