Open mountiny opened 12 months ago
:wave:
@perunt
👋
Also Assigning @AndrewGable as he will most likely help us too 🤝 feel free to unassign if you prefer
Update:
There was again a false positive on the e2e test runs. Added this PR trying to minimise those errors:
⚠️ Looks like this issue was linked to a Deploy Blocker here
If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.
If a regression has occurred and you are the assigned CM follow the instructions here.
If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.
Lies, it was a different pr causing the issue
This issue has not been updated in over 15 days. @AndrewGable, @hannojg, @mountiny, @perunt eroding to Monthly issue.
P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do!
@hannojg @perunt how is it going?
typing
test is implemented and merged!(Note: we are internally working on a system to make the e2e tests even better, more news on that soon! ™️ )
Thanks!
adding myself here for reviewing the chat switching one!
Chat switching is merged 🎊 !
@perunt I think we can add "number of renders when we open chat (it should be 1-2)" to the same test as second metric?
Also next up would be the search page test!
Reviewing
label has been removed, please complete the "BugZero Checklist".
The solution for this issue has been :rocket: deployed to production :rocket: in version 1.4.14-6 and is now subject to a 7-day regression period :calendar:. Here is the list of pull requests that resolve this issue:
If no regressions arise, payment will be issued on 2023-12-28. :confetti_ball:
After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.
For reference, here are some details about the assignees on this issue:
I think we might want to take a better look into how we are using the E2E tests now and make sure we already getting some use of it before adding more scenarios. In theory it can be very powerful tool, but so far we have not really been using the results of the tests to stop any PRs/ deploys. Come more convo here cc @blimpich @roryabraham @iwiznia
@AndrewGable, @dangrous, @hannojg, @mountiny, @perunt Whoops! This issue is 2 days overdue. Let's get this updated quick!
My 2 cents: E2E tests should be used incredibly sparingly. E2E tests are powerful but they're also expensive to make, maintain, and debug. I don't have much context on Expensify's E2E suite but I'd strongly lean in the direction of investing in many small, inexpensive unit tests instead of more E2E tests.
I'd also agree that if we aren't even using our current E2E test suite to stop deploys or cause code freezes, that there is no real reason why we even should care about the E2E test suite. The test suite as it stands is providing little to no value, since we currently can go a week with it being broken in main
and no one noticing (see this slack thread).
Ignore this, I should have brought this up internally, I misunderstood and don't think this was the right place for me to make this comment.
This is not overdue, we are continuing as normal
@AndrewGable, @dangrous, @hannojg, @mountiny, @perunt Huh... This is 4 days overdue. Who can take care of this?
This is currently on hold by the effort to change how we mock the API. Discussion is here:
https://expensify.slack.com/archives/C035J5C9FAP/p1703237225747209
Working on the API mocking
@AndrewGable, @dangrous, @hannojg, @mountiny, @perunt Whoops! This issue is 2 days overdue. Let's get this updated quick!
Solution for the API mocking was provided, we need to test it
@hannojg whats the latest here?
Update: the e2e tests seem to be broken right now and we are working on finding out why. Conversation is happening here.
The e2e tests became broken again, mentioned and fixed here.
We are working on:
Once thats all merged and stable we will work on the next e2e tests (hopefully 🔜 ™️ )
@hannojg monitoring the reliability of existing tests
@hannojg is working on this
After this PR was merged:
we are monitoring stability again, then moving on to implementing more tests!
@hannojg what is the latest on this one?
The tests are semi stable, currently 15% of them fail during running. Still want to iron that out!
@hannojg what are the next steps on this issue from your side?
We are now actively working on implementing the tests that align with the core metrics:
@hannojg how is this looking?
We have two PRs open waiting for final review / clarifications:
@hannojg What's next now?
Next is to allocate time for this task in our team and move forward! Nothing blocking as far as I can see. We'll pick this up again next week!
This issue has not been updated in over 15 days. @dangrous, @hannojg, @perunt eroding to Monthly issue.
P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do!
We were chatting in slack about pointing these E2E tests at staging and using them to help figure out our "high water point" at which the performance degrades critically with an increasing number of reports across various devices.
@roryabraham I remember we discussed doing a one-off report for those "high water points" here
Is this something we should look at?
cc @kirillzyusko
I believe @rinej is working on that
@kirillzyusko lets finish this one!
Which tests are currently missing? I see that we are still missing:
I think for the onyx tests we wanted some tests where we don't load the full app, but just run some performance tests with mock data in onyx to catch performance regressions in onyx.
@hannojg yes, you are right. I think for number of renders we only track amount of re-renders in Composer.
Also I think we'll need to revise the approach for tracking number of re-renders, because now we rely on the fact, that if parent was re-rendered, then children that are not wrapped in memo will be re-rendered. But with react-compiler
it doesn't work anymore, because it'll memoize everything automatically 👀
Also I think we'll need to revise the approach for tracking number of re-renders, because now we rely on the fact, that if parent was re-rendered, then children that are not wrapped in memo will be re-rendered. But with react-compiler it doesn't work anymore, because it'll memoize everything automatically 👀
Just as a proof of my words - below I attached a report (after react-compiler
was merged). And as we can see (first of all it really works 😅) that amount of renders went down from 2 to 0. I assume we have the same amount of re-renders, but since we were relying on an approach described above -> we have modified numbers now, so eventually at some point of time we'll need to revisit that.
There are no entries
hi friends! just checking in - where are we on this one? let me know if I can help in any way!
@dangrous I think we are currently (still) trying to fix e2e tests. And only after that we can start to write new e2e tests 👀
how are we looking?
The E2E tests are now set up and working again, lets expand on their options and add more flows and metrics:
@Szymon20000 @hannojg