Skip to content
This repository has been archived by the owner on Mar 10, 2024. It is now read-only.

fix!: custom object records integration tests #1858

Merged
merged 1 commit into from
Nov 6, 2023

Conversation

asdfryan
Copy link
Contributor

@asdfryan asdfryan commented Nov 3, 2023

This also includes some bugfixes on the relevant code:

  • PATCH should return 200 not 201
  • Missing required fields in hubspot should return 400 not 500

Copy link

vercel bot commented Nov 3, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

2 Ignored Deployments
Name Status Preview Updated (UTC)
management-ui ⬜️ Ignored (Inspect) Nov 3, 2023 11:00pm
supaglue-docs ⬜️ Ignored (Inspect) Nov 3, 2023 11:00pm


// describe.each(['hubspot'])('%s', (providerName) => {
describe.each(['hubspot', 'salesforce'])('%s', (providerName) => {
test(`Post /`, async () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

this is testing quite a bit of things: can you make the test more descriptive or annotate in the test what mutations you're making and what you're testing for?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am using the same pattern / precedent as all the other integration tests.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we improve upon it for readability; I haven't gotten to reviewing every PR so some patterns we can improve upon

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In that case I would like to open a separate PR where I can address this holistically across all of our integration tests. Does that sound good to you?

if (message === 'one or more associations are not valid') {
if (
message === 'one or more associations are not valid' ||
message.includes('Some required properties were not set')
Copy link
Contributor

Choose a reason for hiding this comment

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

startsWith or includes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it says includes here.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm more asking what it should be: it looks like we're expecting it to be the start of the message, and the end of the message is dynamic?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you look at the integration test I added, you can see that the full error message is Error creating {{CUSTOM_OBJ_NAME}}. Some required properties were not set.

@tomkit
Copy link
Contributor

tomkit commented Nov 6, 2023

This should be a fix! PR not a chore

@asdfryan
Copy link
Contributor Author

asdfryan commented Nov 6, 2023

This should be a fix! PR not a chore

There are no breaking changes in this PR.

@asdfryan asdfryan requested a review from tomkit November 6, 2023 22:52
@asdfryan asdfryan changed the title chore: custom object records integration tests fix: custom object records integration tests Nov 6, 2023
@tomkit
Copy link
Contributor

tomkit commented Nov 6, 2023

This should be a fix! PR not a chore

There are no breaking changes in this PR.

The response code change is breaking

@asdfryan asdfryan changed the title fix: custom object records integration tests fix!: custom object records integration tests Nov 6, 2023
@asdfryan asdfryan merged commit db8a687 into main Nov 6, 2023
13 checks passed
@asdfryan asdfryan deleted the custom_object_records_test branch November 6, 2023 23:32
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants