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

[HOLD for payment 2024-11-11] [HOLD for payment 2024-11-01] E2E Testing: PRs merged after a regression all get marked as regression as well #48085

Closed
hannojg opened this issue Aug 27, 2024 · 21 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Weekly KSv2

Comments

@hannojg
Copy link
Contributor

hannojg commented Aug 27, 2024

With the current architecture of the e2e tests there is a problem where:

  • A new release is created, this will be the baseline for the e2e performance regression testing
  • PR1 is merged which introduces a performance regression
  • PR2 is merged with no performance regression
  • PR3 is merged with no performance regression

however, all PRs will be compared against the same baseline. Thus not only PR1 gets marked as deploy blocker for having introduced a performance regression, but also PR2 and PR3.

This creates confusion and additional spam in slack:

Screenshot 2024-08-27 at 15 04 46

As discussed here, the proposed solution is to create for each merge commit to make a new baseline build from the previous merge commit and compare it against that one.

@hannojg
Copy link
Contributor Author

hannojg commented Aug 27, 2024

cc @kirillzyusko can you comment here so it can be assigned to you? Thanks!

@mountiny mountiny self-assigned this Aug 27, 2024
@mountiny
Copy link
Contributor

cc @AndrewGable does this sound good to you?

@AndrewGable
Copy link
Contributor

Yes sounds great!

@muttmuure muttmuure moved this to MEDIUM in [#whatsnext] #quality Aug 27, 2024
@mountiny
Copy link
Contributor

@kirillzyusko, are you going to take this one on? Can you comment here if so? thanks

@kirillzyusko
Copy link
Contributor

Yeah, feel free to assign this to me 🙌

@mountiny
Copy link
Contributor

mountiny commented Sep 6, 2024

not overdue

@melvin-bot melvin-bot bot removed the Overdue label Sep 6, 2024
@kirillzyusko
Copy link
Contributor

I think we discussed this internally with Hanno, but I'll post here as well.

We think that we should postpone this PR, because current e2e tests are not very stable (i. e. sometimes they pass, sometimes not) and having such unstable pipeline with assembling builds for each new commit may lead to uncaught regressions, i. e.:

A <- first commit in chain, e2e passed no regression has been detected
|
|
B <- commit that introduces a regression and e2e tests fails
|
|
C <- e2e tests pass again, but we compare with a build that introduced a regression and this regression was unnoticed

So I think we need to have a good e2e pipeline first and only then go ahead with this PR 👀

@mountiny
Copy link
Contributor

Sounds good, are you or someone else actively working on fixing the pipeline?

@kirillzyusko
Copy link
Contributor

Sounds good, are you or someone else actively working on fixing the pipeline?

@mountiny current e2e pipeline slightly unstable, but we need to have gather logs/videos for all latest failures to detect what needs to be fixed first.

From what I saw - we have several major problems:

  • sometimes tests fail after 2-3 mins - looks like a job can not be started properly;
  • sometimes tests fail after 9-10 minutes - it means that something is wrong with first test;
  • sometimes third test fails;

I think we need to gather kind of analytics on what is the most frequent failure, gather logs for this failure and try to fix it. How does it sound for you?

I can prepare analytic based on latest failures (and attach links for all of them) but I need someone to help me to get logs + videos. Would you be able to help me? 👀

@mountiny
Copy link
Contributor

Provided some more logs in DM and also created an issue to give you and some other people from Margelo access to the device farm

@melvin-bot melvin-bot bot added the Overdue label Sep 24, 2024
@mountiny
Copy link
Contributor

@kirillzyusko how is this looking

@melvin-bot melvin-bot bot removed the Overdue label Sep 27, 2024
@kirillzyusko
Copy link
Contributor

@mountiny I prepared two PRs:

The remaining problem will be AWS scheduling - my assumption is that we'll need to add retry mechanism to that step (I think we can try to use https://github.com/marketplace/actions/retry-action).

But overall with these 2 fixes tests should become pretty stable (we should have 1-2 failures per week, which I think is kind of acceptable).

@mountiny
Copy link
Contributor

mountiny commented Oct 7, 2024

Bumped the PRs

@muttmuure muttmuure moved this from MEDIUM to HIGH in [#whatsnext] #quality Oct 15, 2024
@melvin-bot melvin-bot bot added the Overdue label Oct 15, 2024
@mountiny
Copy link
Contributor

How is this looking? Can @chrispader help here? I read he has some spare cycles

@melvin-bot melvin-bot bot removed the Overdue label Oct 16, 2024
@kirillzyusko
Copy link
Contributor

@mountiny I will work on it 👀 Just will finish react-native-app-logs and will switch back to e2e tests stuff!

@melvin-bot melvin-bot bot added Reviewing Has a PR in review and removed Weekly KSv2 labels Oct 21, 2024
@melvin-bot melvin-bot bot added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Weekly KSv2 labels Oct 21, 2024
@melvin-bot melvin-bot bot changed the title E2E Testing: PRs merged after a regression all get marked as regression as well [HOLD for payment 2024-11-01] E2E Testing: PRs merged after a regression all get marked as regression as well Oct 25, 2024
Copy link

melvin-bot bot commented Oct 25, 2024

Reviewing label has been removed, please complete the "BugZero Checklist".

@melvin-bot melvin-bot bot removed the Reviewing Has a PR in review label Oct 25, 2024
Copy link

melvin-bot bot commented Oct 25, 2024

The solution for this issue has been 🚀 deployed to production 🚀 in version 9.0.53-1 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

If no regressions arise, payment will be issued on 2024-11-01. 🎊

For reference, here are some details about the assignees on this issue:

Copy link

melvin-bot bot commented Nov 1, 2024

Skipping the payment summary for this issue since all the assignees are employees or vendors. If this is incorrect, please manually add the payment summary SO.

@melvin-bot melvin-bot bot added Overdue Weekly KSv2 and removed Daily KSv2 labels Nov 4, 2024
@melvin-bot melvin-bot bot changed the title [HOLD for payment 2024-11-01] E2E Testing: PRs merged after a regression all get marked as regression as well [HOLD for payment 2024-11-11] [HOLD for payment 2024-11-01] E2E Testing: PRs merged after a regression all get marked as regression as well Nov 4, 2024
@melvin-bot melvin-bot bot removed the Overdue label Nov 4, 2024
Copy link

melvin-bot bot commented Nov 4, 2024

The solution for this issue has been 🚀 deployed to production 🚀 in version 9.0.56-9 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

If no regressions arise, payment will be issued on 2024-11-11. 🎊

For reference, here are some details about the assignees on this issue:

Copy link

melvin-bot bot commented Nov 11, 2024

Skipping the payment summary for this issue since all the assignees are employees or vendors. If this is incorrect, please manually add the payment summary SO.

@mountiny
Copy link
Contributor

This was a follow up to improve the E2E test suite, I dont think we need a checklist in this case as Margelo keeps slowly improving the suite. No external review was done so no payment required either

@github-project-automation github-project-automation bot moved this from HIGH to Done in [#whatsnext] #quality Nov 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Weekly KSv2
Projects
Development

No branches or pull requests

4 participants