-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
[Server Integration tests] Enrich integration GraphQL API tests #3 #7931
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR Summary
This pull request adds comprehensive integration tests for various GraphQL resolvers in the Twenty server. Here are the key points:
- Introduced new integration tests for message threads, connected accounts, favorites, notes, opportunities, and other related entities.
- Tests cover CRUD operations (create, read, update, delete) for both single and multiple records.
- Implemented soft deletion and permanent destruction scenarios for each entity.
- Added tests for filtering and relationship handling between entities.
- Ensured test independence by creating and destroying necessary records within each test suite.
11 file(s) reviewed, 9 comment(s)
Edit PR Review Bot Settings | Greptile
@@ -0,0 +1 @@ | |||
export const TIM_ACCOUNT_ID = '20202020-0687-4c41-b707-ed1bfca972a7'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: Consider adding a comment explaining the purpose of this constant and its usage in tests
calendarEventParticipantDisplayName, | ||
); | ||
|
||
expect(createdCalendarEventParticipant).toHaveProperty('displayName'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: Duplicate assertion for 'displayName' property
const CONNECTED_ACCOUNT_1_ID = '777a8457-eb2d-40ac-a707-551b615b6987'; | ||
const CONNECTED_ACCOUNT_2_ID = '777a8457-eb2d-40ac-a707-551b615b6988'; | ||
const CONNECTED_ACCOUNT_3_ID = '777a8457-eb2d-40ac-a707-551b615b6989'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: Consider using a more descriptive naming convention for test IDs
const FAVORITE_1_ID = '777a8457-eb2d-40ac-a707-551b615b6987'; | ||
const FAVORITE_2_ID = '777a8457-eb2d-40ac-a707-551b615b6988'; | ||
const FAVORITE_3_ID = '777a8457-eb2d-40ac-a707-551b615b6989'; | ||
const INITIAL_FAVORITE_POSITION_1 = 1111111; | ||
const INITIAL_FAVORITE_POSITION_2 = 2222222; | ||
const INITIAL_FAVORITE_POSITION_3 = 3333333; | ||
const NEW_FAVORITE_POSITION_1 = 4444444; | ||
const NEW_FAVORITE_POSITION_2 = 5555555; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: Consider using a more descriptive naming convention for constants, e.g., FAVORITE_1_UUID instead of FAVORITE_1_ID
|
||
const response = await makeGraphqlAPIRequest(graphqlOperation); | ||
|
||
const updatedfavorites = response.body.data.updateFavorites; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: updatedfavorites should be updatedFavorites for consistency with camelCase naming
|
||
const response = await makeGraphqlAPIRequest(graphqlOperation); | ||
|
||
const updatedfavorite = response.body.data.updateFavorite; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: updatedfavorite should be updatedFavorite for consistency with camelCase naming
const destroyFavoriteResponse = | ||
await makeGraphqlAPIRequest(graphqlOperation); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: Consider using async/await consistently instead of mixing with .then() style
|
||
const response = await makeGraphqlAPIRequest(graphqlOperation); | ||
|
||
const updatedmessageChannels = response.body.data.updateMessageChannels; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: Variable name should be camelCase
expect(createdMessageParticipant).toHaveProperty('deletedAt'); | ||
}); | ||
|
||
it('2. should find many messageParticipants', async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: Check if edges exist before accessing first element
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gitstart-twenty Thank you! LGTM
This PR was created by GitStart to address the requirements from this ticket: TWNTY-7526.
Description
NoteTargets and MessageThreads are special cases because they do not have a notable property that we could use in "update" the test cases,
for NoteTargets we are using the personId, testing the relation, but for MessageThreads we are using updatedAt. To test the relations for MessageThreads
we would need to update another object (Message) because the relation is ONE_TO_MANY, updating another object in a test that would update the current tested object sounds incorrect.
In the NoteTargets, we can update the NoteTarget object because the relation is MANY_TO_ONE
for some tests we need an account ID, we are using Tim's account for all the tests (the token in jest-integration.config.ts), so we are using a constant to use the account ID
Refs
#7526
Demo