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

chore: stricter self-review guideline #9327

Merged
merged 2 commits into from
Jun 5, 2023

Conversation

hariombalhara
Copy link
Member

Adds stricter self-review guideline

@vercel
Copy link

vercel bot commented Jun 5, 2023

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

Name Status Preview Comments Updated (UTC)
cal ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jun 5, 2023 10:05am
ui ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jun 5, 2023 10:05am
1 Ignored Deployment
Name Status Preview Comments Updated (UTC)
api ⬜️ Ignored (Inspect) Jun 5, 2023 10:05am

@@ -8,8 +8,6 @@ Fixes # (issue)
Loom Video: https://www.loom.com/
-->

**Environment**: Staging(main branch) / Production
Copy link
Member Author

Choose a reason for hiding this comment

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

Discussed earlier in group that this isn't needed as we do patch releases every other day and main is almost always the production.

@@ -27,13 +25,14 @@ Fixes # (issue)
- [ ] Test A
- [ ] Test B

## Mandatory Tasks
- [ ] Make sure you have self-reviewed the code. A decent size PR without self-review might be rejected.
Copy link
Member Author

Choose a reason for hiding this comment

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

Moves the self-review guideline to a new place so that it's noticed immediately. Also, added under a stricter headline.

@github-actions
Copy link
Contributor

github-actions bot commented Jun 5, 2023

📦 Next.js Bundle Analysis for @calcom/web

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

One Page Changed Size

The following page changed size from the code in this PR compared to its base branch:

Page Size (compressed) First Load % of Budget (350 KB)
/apps/[slug]/[...pages] 459.4 KB 610.61 KB 174.46% (🟢 -0.19%)
Details

Only the gzipped size is provided here based on an expert tip.

First Load is the size of the global bundle plus the bundle for the individual page. If a user were to show up to your website and land on a given page, the first load size represents the amount of javascript that user would need to download. If next/link is used, subsequent page loads would only need to download that page's bundle (the number in the "Size" column), since the global bundle has already been downloaded.

Any third party scripts you have added directly to your app using the <script> tag are not accounted for in this analysis

The "Budget %" column shows what percentage of your performance budget the First Load total takes up. For example, if your budget was 100kb, and a given page's first load size was 10kb, it would be 10% of your budget. You can also see how much this has increased or decreased compared to the base branch of your PR. If this percentage has increased by 20% or more, there will be a red status indicator applied, indicating that special attention should be given to this. If you see "+/- <0.01%" it means that there was a change in bundle size, but it is a trivial enough amount that it can be ignored.

@deploysentinel
Copy link

deploysentinel bot commented Jun 5, 2023

Current Playwright Test Results Summary

✅ 82 Passing - ❌ 15 Failing - ⚠️ 1 Flaky

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

(Last updated on 06/05/2023 09:02:09am UTC)

Run Details

Running Workflow PR Update on Github Actions

Commit: b5fd01a

Started: 06/05/2023 08:55:31am UTC

❌ Failures

📄   apps/web/playwright/booking-pages.e2e.ts • 3 Failures

Top 1 Common Error Messages

Test timeout of 60000ms exceeded.

3 Test Cases Affected

Test Case Results

Test Case Last 7 days Failures Last 7 days Flakes
free user -- old-booker cannot book same slot multiple times
Retry 2Retry 1Initial Attempt
Error: Test timeout of 60000ms exceeded.
Test timeout of 60000ms exceeded.
0.76% (3) 3 / 395 runs
failed over last 7 days
0% (0) 0 / 395 runs
flaked over last 7 days
pro user -- old-booker book an event first day in next month
Retry 2Retry 1Initial Attempt
Error: Test timeout of 60000ms exceeded.
Test timeout of 60000ms exceeded.
0.78% (3) 3 / 384 runs
failed over last 7 days
0% (0) 0 / 384 runs
flaked over last 7 days
pro user -- old-booker can reschedule a booking
Retry 2Retry 1Initial Attempt
Error: Test timeout of 60000ms exceeded.
Test timeout of 60000ms exceeded.
0.78% (3) 3 / 383 runs
failed over last 7 days
0% (0) 0 / 383 runs
flaked over last 7 days

📄   apps/web/playwright/webhook.e2e.ts • 3 Failures

Top 1 Common Error Messages

Test timeout of 60000ms exceeded.

3 Test Cases Affected

Test Case Results

Test Case Last 7 days Failures Last 7 days Flakes
BOOKING_CREATED add webhook & test that creating an event triggers a webhook call
Retry 2Retry 1Initial Attempt
Error: Test timeout of 60000ms exceeded.
Test timeout of 60000ms exceeded.
2.32% (6) 6 / 259 runs
failed over last 7 days
0.39% (1) 1 / 259 run
flaked over last 7 days
BOOKING_REJECTED can book an event that requires confirmation and then that booking can be rejected by organizer
Retry 2Retry 1Initial Attempt
Error: Test timeout of 60000ms exceeded.
Test timeout of 60000ms exceeded.
2.32% (6) 6 / 259 runs
failed over last 7 days
1.16% (3) 3 / 259 runs
flaked over last 7 days
BOOKING_REQUESTED can book an event that requires confirmation and get a booking requested event
Retry 2Retry 1Initial Attempt
Error: Test timeout of 60000ms exceeded.
Test timeout of 60000ms exceeded.
1.94% (5) 5 / 258 runs
failed over last 7 days
0% (0) 0 / 258 runs
flaked over last 7 days

📄   apps/web/playwright/booking-seats.e2e.ts • 3 Failures

Top 1 Common Error Messages

Test timeout of 60000ms exceeded.

3 Test Cases Affected

Test Case Results

Test Case Last 7 days Failures Last 7 days Flakes
Booking with Seats -- old-booker Multiple Attendees can book a seated event time slot
Retry 2Retry 1Initial Attempt
Error: Test timeout of 60000ms exceeded.
Test timeout of 60000ms exceeded.
0.77% (3) 3 / 388 runs
failed over last 7 days
0% (0) 0 / 388 runs
flaked over last 7 days
Reschedule for booking with seats -- old-booker Should reschedule booking with seats
Retry 2Retry 1Initial Attempt
Error: Test timeout of 60000ms exceeded.
Test timeout of 60000ms exceeded.
1.02% (3) 3 / 294 runs
failed over last 7 days
0.34% (1) 1 / 294 run
flaked over last 7 days
Reschedule for booking with seats -- old-booker Should reschedule booking with seats and if everyone rescheduled it should be deleted
Retry 2Retry 1Initial Attempt
Error: Test timeout of 60000ms exceeded.
Test timeout of 60000ms exceeded.
1.02% (3) 3 / 294 runs
failed over last 7 days
10.20% (30) 30 / 294 runs
flaked over last 7 days

📄   apps/web/playwright/reschedule.e2e.ts • 2 Failures

Top 1 Common Error Messages

Test timeout of 60000ms exceeded.

2 Test Cases Affected

Test Case Results

Test Case Last 7 days Failures Last 7 days Flakes
Reschedule Tests -- old-booker Should display former time when rescheduling availability
Retry 2Retry 1Initial Attempt
Error: Test timeout of 60000ms exceeded.
Test timeout of 60000ms exceeded.
0.27% (1) 1 / 370 run
failed over last 7 days
0% (0) 0 / 370 runs
flaked over last 7 days
Reschedule Tests -- old-booker Should do a reschedule from user owner
Retry 2Retry 1Initial Attempt
Error: Test timeout of 60000ms exceeded.
Test timeout of 60000ms exceeded.
0.81% (3) 3 / 370 runs
failed over last 7 days
0% (0) 0 / 370 runs
flaked over last 7 days

📄   packages/embeds/embed-core/playwright/tests/action-based.e2e.ts • 2 Failures

Top 1 Common Error Messages

Test timeout of 60000ms exceeded.

2 Test Cases Affected

Test Case Results

Test Case Last 7 days Failures Last 7 days Flakes
Popup Tests should open embed iframe on click - Configured with light theme
Retry 2Retry 1Initial Attempt
Error: Test timeout of 60000ms exceeded.
Test timeout of 60000ms exceeded.
1.48% (3) 3 / 203 runs
failed over last 7 days
7.39% (15) 15 / 203 runs
flaked over last 7 days
Popup Tests should be able to reschedule
Retry 2Retry 1Initial Attempt
Error: Test timeout of 60000ms exceeded.
Test timeout of 60000ms exceeded.
3.94% (8) 8 / 203 runs
failed over last 7 days
93.10% (189) 189 / 203 runs
flaked over last 7 days

📄   apps/web/playwright/manage-booking-questions.e2e.ts • 1 Failure

Test Case Results

Test Case Last 7 days Failures Last 7 days Flakes
Manage Booking Questions For User EventType -- old-booker Do a booking with a user added question and verify a few thing in b/w
Retry 1Initial Attempt
Error: Test timeout of 180000ms exceeded.
Test timeout of 180000ms exceeded.
2.33% (9) 9 / 387 runs
failed over last 7 days
0% (0) 0 / 387 runs
flaked over last 7 days

📄   packages/embeds/embed-core/playwright/tests/inline.e2e.ts • 1 Failure

Test Case Results

Test Case Last 7 days Failures Last 7 days Flakes
Inline Iframe - Configured with Dark Theme
Retry 2Retry 1Initial Attempt
Error: Test timeout of 60000ms exceeded.
Test timeout of 60000ms exceeded.
2.94% (6) 6 / 204 runs
failed over last 7 days
0% (0) 0 / 204 runs
flaked over last 7 days

⚠️ Flakes

📄   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
1.32% (5) 5 / 378 runs
failed over last 7 days
19.84% (75) 75 / 378 runs
flaked over last 7 days

View Detailed Build Results


@emrysal emrysal enabled auto-merge June 5, 2023 09:53
@baileypumfleet baileypumfleet merged commit 6aa8310 into main Jun 5, 2023
6 of 8 checks passed
@baileypumfleet baileypumfleet deleted the chore/stricter-self-review-guideline branch June 5, 2023 10:00
@muescha
Copy link

muescha commented Jun 5, 2023

Are there any visible tasks that need to be completed by the PR author?
How can you determine if a PR has not undergone self-review?
Maybe we provide a link that offers a detailed explanation of what it means for a PR to be "self-reviewed"?

@hariombalhara
Copy link
Member Author

Yeah that makes total sense. This is my understanding of the task.

  • Go through each of the file keeping in mind that the reviewer hasn't worked together with you on that PR but he would have some context of what's going on.
  • If it's something that would be useful say 3-4 months down the line, then it should probably be a comment in the code itself(and not the PR). It might be a README change as well.
  • Add comments at appropriate place in the code
  • Once done, go to Review and write a comment like 'Self review complete.'

@PeerRich PeerRich added the core area: core, team members only label Jul 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core area: core, team members only
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants