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

test: Integration tests for handleNewBooking #11044

Merged
merged 23 commits into from
Sep 6, 2023
Merged

Conversation

hariombalhara
Copy link
Member

@hariombalhara hariombalhara commented Aug 31, 2023

What does this PR do?

Note: There is some refactor in the stacked PR. I would be implementing any suggestions there to avoid non trivial conflicts

Adds integration tests for handleNewBooking. The tests are added in such a way that it would allow us to refactor the booking flow with lesser worries. I expect not much maintenance to be required for the tests as they mock the constructs that are going to stay there unless we do something drastic.
Screenshot 2023-09-02 at 3 37 19 PM

Partially completes #9284. Now the refactor can be done with a bit more confidence.

This was done to work on #7120 with more confidence.

Detailed Changes

  • Abstract out booking tests related code to bookingScenario so that it can be shared b/w getSchedule.test.ts and handleNewBooking.test.ts
  • Upgraded Vitest to be able to use fixture
  • Added vitest-fetch-mock to mock webhook fetch calls.

Following things are mocked

  • AppStore CalendarService - We just verify that the respective app's appropriate method has been called.
    • Because it's mocked, Calendar Apps should test their logic themeselves.
  • AppStore VideoApiAdapter - We just verify that the respective app's appropriate method has been called.
    • Because it's mocked, Calendar Apps should test their logic themeselves.
  • fetch to verify webhooks
  • Prisma queries

Run the tests as follows

Watch mode: yarn tdd handleNewBooking
One time: yarn test handleNewBooking

More improvements in the stacked PR #11099

@vercel
Copy link

vercel bot commented Aug 31, 2023

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

Name Status Preview Comments Updated (UTC)
ai ❌ Failed (Inspect) Sep 6, 2023 7:16pm
api ✅ Ready (Inspect) Visit Preview 💬 Add feedback Sep 6, 2023 7:16pm
cal-demo ✅ Ready (Inspect) Visit Preview 💬 Add feedback Sep 6, 2023 7:16pm
dev ✅ Ready (Inspect) Visit Preview 💬 Add feedback Sep 6, 2023 7:16pm
ui ✅ Ready (Inspect) Visit Preview 💬 Add feedback Sep 6, 2023 7:16pm
2 Ignored Deployments
Name Status Preview Comments Updated (UTC)
cal ⬜️ Ignored (Inspect) Visit Preview Sep 6, 2023 7:16pm
qa ⬜️ Ignored (Inspect) Visit Preview Sep 6, 2023 7:16pm

@github-actions
Copy link
Contributor

github-actions bot commented Aug 31, 2023

Thank you for following the naming conventions! 🙏

@zomars zomars added the core area: core, team members only label Aug 31, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Aug 31, 2023

📦 Next.js Bundle Analysis for @calcom/web

This analysis was generated by the Next.js Bundle Analysis action. 🤖

This PR introduced no changes to the JavaScript bundle! 🙌

@deploysentinel
Copy link

deploysentinel bot commented Aug 31, 2023

Current Playwright Test Results Summary

✅ 117 Passing - ⚠️ 5 Flaky

Run may still be in progress, this comment will be updated as current testing workflow or job completes...

(Last updated on 09/06/2023 07:17:33pm UTC)

Run Details

Running Workflow PR Update on Github Actions

Commit: 983ab2c

Started: 09/06/2023 07:14:14pm UTC

⚠️ Flakes

📄   apps/web/playwright/login.2fa.e2e.ts • 1 Flake

Test Case Results

Test Case Last 7 days Failures Last 7 days Flakes
2FA Tests should allow a user to enable 2FA and login using 2FA
Retry 1Initial Attempt
0.67% (2) 2 / 298 runs
failed over last 7 days
27.85% (83) 83 / 298 runs
flaked over last 7 days

📄   apps/web/playwright/integrations-stripe.e2e.ts • 1 Flake

Test Case Results

Test Case Last 7 days Failures Last 7 days Flakes
Stripe integration Pending payment booking should not be confirmed by default
Retry 1Initial Attempt
1.63% (5) 5 / 307 runs
failed over last 7 days
4.89% (15) 15 / 307 runs
flaked over last 7 days

📄   apps/web/playwright/event-types.e2e.ts • 1 Flake

Test Case Results

Test Case Last 7 days Failures Last 7 days Flakes
Event Types tests user Different Locations Tests can add Attendee Phone Number location and book with it
Retry 1Initial Attempt
1.98% (6) 6 / 303 runs
failed over last 7 days
10.89% (33) 33 / 303 runs
flaked over last 7 days

📄   apps/web/playwright/managed-event-types.e2e.ts • 1 Flake

Test Case Results

Test Case Last 7 days Failures Last 7 days Flakes
Managed Event Types tests Can create managed event type
Retry 1Initial Attempt
3.18% (10) 10 / 314 runs
failed over last 7 days
11.46% (36) 36 / 314 runs
flaked over last 7 days

📄   packages/embeds/embed-core/playwright/tests/action-based.e2e.ts • 1 Flake

Test Case Results

Test Case Last 7 days Failures Last 7 days Flakes
Popup Tests should be able to reschedule
Retry 1Initial Attempt
1.93% (6) 6 / 311 runs
failed over last 7 days
96.78% (301) 301 / 311 runs
flaked over last 7 days

View Detailed Build Results


Copy link
Contributor

@shivamklr shivamklr left a comment

Choose a reason for hiding this comment

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

Added a few changes, let me know your thoughts on the same.

};
});

logger.silly("TestData: Creating EventType", eventTypes);
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure how these logs are helpful with the current logger implementation, https://github.com/calcom/cal.com/blob/2f90dc7cb6470fea486d19dce5de9714a0b9c079/packages/lib/logger.ts#L6C1-L6C64

I must be missing a .env variable, let me know if that is the case.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah right now you need to set the log level manually in code like this
logger.setSettings({ minLevel: "silly" }). Don't want to hardcode this, maybe I can pass some argument to vitest and then based on that I can enable this log level, so that it doesn't spam by default.

Copy link
Contributor

Choose a reason for hiding this comment

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

We can maybe use an .env variable to set the logger level and keep the default to warn and error level.

Copy link
Contributor

Choose a reason for hiding this comment

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

We should keep the types in the separate declaration file.

Copy link
Contributor

Choose a reason for hiding this comment

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

yarn test getSchedule is failing locally. Kindly let me know if I setup something wrong.
getScheduleTestError

Copy link
Member Author

Choose a reason for hiding this comment

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

I think getSchedule is known to fail depending on timezones. For now, if it's passing on CI, we should be fine.

Copy link
Contributor

Choose a reason for hiding this comment

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

@emrysal What do you think?

@hariombalhara
Copy link
Member Author

hariombalhara commented Sep 6, 2023

Thanks @shivamklr for the detailed review 🙏 Addressed all your feedback here in the stacked PR

@@ -0,0 +1,511 @@
/**
* How to ensure that unmocked prisma queries aren't called?
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

For now I have ensured that through fallbackMockImplementation

Copy link
Member

@zomars zomars left a comment

Choose a reason for hiding this comment

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

Awesome work @hariombalhara

Looks ready to merge

Ship it

@zomars zomars merged commit f9eb335 into main Sep 6, 2023
@zomars zomars deleted the test/handleNewBooking branch September 6, 2023 19:23
sean-brydon pushed a commit that referenced this pull request Sep 7, 2023
Co-authored-by: Shivam Kalra <[email protected]>
Co-authored-by: Joe Au-Yeung <[email protected]>
sean-brydon added a commit that referenced this pull request Sep 7, 2023
* Pass organization name & logo

* Overflow hidden

* Show org icon on public page

* Add org logo to large user avatars

* Clean up

* Add org name and logo to context

* Get org logo from /avatar.png endpoint

* Do not query for logo

* Remove name and logo from session middleware

* Type fix

* Set user onboarding org logo

* feat: organization avatar component (#10788)

Co-authored-by: sean-brydon <[email protected]>

* Type fixes

* Type fix

* Transition to org slug for organization avatar

* Address feedback

* Clean up

* Clean up

* Type fix

* fix: set avatar cache control (#11163)

* test: Integration tests for handleNewBooking (#11044)

Co-authored-by: Shivam Kalra <[email protected]>
Co-authored-by: Joe Au-Yeung <[email protected]>

* fix: booking_paid webhook and added new payment metadata (#11093)

* app store improvements, logos, dark mode, added screenshots, fixed author names (#11164)

* fix: mobile event types and avatars (#11184)

* New Crowdin translations by Github Action

* fix: updateProfile metadata overwrite (#11188)

Co-authored-by: alannnc <[email protected]>

* New Crowdin translations by Github Action

---------

Co-authored-by: Sean Brydon <[email protected]>
Co-authored-by: sean-brydon <[email protected]>
Co-authored-by: Omar López <[email protected]>
Co-authored-by: Hariom Balhara <[email protected]>
Co-authored-by: Shivam Kalra <[email protected]>
Co-authored-by: alannnc <[email protected]>
Co-authored-by: Peer Richelsen <[email protected]>
Co-authored-by: Leo Giovanetti <[email protected]>
Co-authored-by: Crowdin Bot <[email protected]>
ashwintelmore pushed a commit to ashwintelmore/cal.com that referenced this pull request Sep 19, 2023
* Pass organization name & logo

* Overflow hidden

* Show org icon on public page

* Add org logo to large user avatars

* Clean up

* Add org name and logo to context

* Get org logo from /avatar.png endpoint

* Do not query for logo

* Remove name and logo from session middleware

* Type fix

* Set user onboarding org logo

* feat: organization avatar component (calcom#10788)

Co-authored-by: sean-brydon <[email protected]>

* Type fixes

* Type fix

* Transition to org slug for organization avatar

* Address feedback

* Clean up

* Clean up

* Type fix

* fix: set avatar cache control (calcom#11163)

* test: Integration tests for handleNewBooking (calcom#11044)

Co-authored-by: Shivam Kalra <[email protected]>
Co-authored-by: Joe Au-Yeung <[email protected]>

* fix: booking_paid webhook and added new payment metadata (calcom#11093)

* app store improvements, logos, dark mode, added screenshots, fixed author names (calcom#11164)

* fix: mobile event types and avatars (calcom#11184)

* New Crowdin translations by Github Action

* fix: updateProfile metadata overwrite (calcom#11188)

Co-authored-by: alannnc <[email protected]>

* New Crowdin translations by Github Action

---------

Co-authored-by: Sean Brydon <[email protected]>
Co-authored-by: sean-brydon <[email protected]>
Co-authored-by: Omar López <[email protected]>
Co-authored-by: Hariom Balhara <[email protected]>
Co-authored-by: Shivam Kalra <[email protected]>
Co-authored-by: alannnc <[email protected]>
Co-authored-by: Peer Richelsen <[email protected]>
Co-authored-by: Leo Giovanetti <[email protected]>
Co-authored-by: Crowdin Bot <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automated-tests area: unit tests, e2e tests, playwright core area: core, team members only High priority Created by Linear-GitHub Sync
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants