-
Notifications
You must be signed in to change notification settings - Fork 325
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
[WPB-5687] more legalhold tests #3966
Conversation
I have completely removed |
I don't think we need a further changelog entry as this is a follow up PR. |
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.
Why do all these tests need a dynamic backend?
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.
I also see that you left some of the LH tests in the old test suite, why so?
because otherwise the rest of the services don't see the mock LH service; perhaps they don't need it though, might just be a skill issue on my behalf :3 do you have a pointer at some place where this is done without dynamic backend?
they don't flake. |
If you try to access
Given these flakes are for galley trying to talk to the dynamic service that we spin up, I think they might flake anyway. So, instead of going through this cycle of moving them in batches, IMO it is best if we move all of them. |
it's still 1400 lines of code to port, this is work that gets tedious after a while and it's harder for concentration, I think this work might benefit from doing in batches ordered by urgency. |
Sure, IMO the urgency is the same, which tests fail is not consistent across runs. If you find it easier to do it in batches, that's fine too. I think we shouldn't close the ticket until all of these are migrated away. |
That’s fine, let’s keep it open. These tests should be the ones that are in the hundreds of failures, not the ones that only failed recently. |
7cee1c6
to
c0de53d
Compare
84f269c
to
aa979d3
Compare
7d4ff6a
to
79aea2e
Compare
cc65a4b
to
855fdb5
Compare
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.
looks good! please consider my nit-picks before you merge.
I have applied your suggestions @fisx with some slight deviations from your doc proposals. |
f9cb4e6
to
7b898f7
Compare
- [feat] port more legalhold tests to /integration - [feat] introduce combinators for lazy Notifications in App --------- Co-authored-by: Marko Dimjašević <marko.dimjasevic@wire.com> Co-authored-by: Igor Ranieri <igor@elland.me> Co-authored-by: Akshay Mankar <akshay@wire.com>
https://wearezeta.atlassian.net/browse/WPB-5687
it appears I had overlooked to port some of the legalhold tests that were still failing
Checklist
changelog.d