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

[WPB-5687] more legalhold tests #3966

Merged
merged 21 commits into from
Apr 10, 2024
Merged

[WPB-5687] more legalhold tests #3966

merged 21 commits into from
Apr 10, 2024

Conversation

MangoIV
Copy link
Contributor

@MangoIV MangoIV commented Mar 19, 2024

https://wearezeta.atlassian.net/browse/WPB-5687
it appears I had overlooked to port some of the legalhold tests that were still failing

  • legalhold status in user profile
  • "Cannot create conversation with both LH activated and non-consenting users" testCannotCreateGroupWithUsersInConflict,
  • image

Checklist

  • Add a new entry in an appropriate subdirectory of changelog.d
  • Read and follow the PR guidelines

@zebot zebot added the ok-to-test Approved for running tests in CI, overrides not-ok-to-test if both labels exist label Mar 19, 2024
@MangoIV
Copy link
Contributor Author

MangoIV commented Mar 20, 2024

I have completely removed testGroupConvInvitationHandlesLHConflicts as it was a dejavu, this is tested multiple times across the file, I will port it if I missed something and this tests otherwise untested behaviour but I think this is the same as in other tests.

@MangoIV MangoIV marked this pull request as ready for review March 25, 2024 09:50
@MangoIV
Copy link
Contributor Author

MangoIV commented Mar 25, 2024

I don't think we need a further changelog entry as this is a follow up PR.

@MangoIV MangoIV changed the title more legalhold tests [WPB-5687] more legalhold tests Mar 25, 2024
Copy link
Member

@akshaymankar akshaymankar left a 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?

Copy link
Member

@akshaymankar akshaymankar left a 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?

integration/test/Notifications.hs Outdated Show resolved Hide resolved
integration/test/Test/LegalHold.hs Outdated Show resolved Hide resolved
@MangoIV
Copy link
Contributor Author

MangoIV commented Mar 25, 2024

Why do all these tests need a dynamic backend?

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?

I also see that you left some of the LH tests in the old test suite, why so?

they don't flake.

@akshaymankar
Copy link
Member

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?

If you try to access wire-server-integration-integration.<namespace>.pod.cluster.local from brig/galley it should get to any port listening in the integration test pod.

they don't flake.

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.

@MangoIV
Copy link
Contributor Author

MangoIV commented Mar 25, 2024

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.

@akshaymankar
Copy link
Member

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.

@MangoIV
Copy link
Contributor Author

MangoIV commented Mar 25, 2024

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.

@elland elland force-pushed the mangoiv/more-legalhold-tests branch 2 times, most recently from 7cee1c6 to c0de53d Compare April 3, 2024 09:39
@mdimjasevic mdimjasevic force-pushed the mangoiv/more-legalhold-tests branch from 84f269c to aa979d3 Compare April 4, 2024 14:17
@MangoIV MangoIV force-pushed the mangoiv/more-legalhold-tests branch from 7d4ff6a to 79aea2e Compare April 8, 2024 10:27
@MangoIV MangoIV force-pushed the mangoiv/more-legalhold-tests branch from cc65a4b to 855fdb5 Compare April 8, 2024 11:00
Copy link
Contributor

@fisx fisx left a 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.

integration/test/Testlib/MockIntegrationService.hs Outdated Show resolved Hide resolved
integration/test/Testlib/MockIntegrationService.hs Outdated Show resolved Hide resolved
integration/test/Notifications.hs Outdated Show resolved Hide resolved
integration/test/Notifications.hs Outdated Show resolved Hide resolved
integration/test/Test/LegalHold.hs Outdated Show resolved Hide resolved
integration/test/Test/LegalHold.hs Show resolved Hide resolved
@MangoIV
Copy link
Contributor Author

MangoIV commented Apr 9, 2024

I have applied your suggestions @fisx with some slight deviations from your doc proposals.

@MangoIV MangoIV force-pushed the mangoiv/more-legalhold-tests branch from f9cb4e6 to 7b898f7 Compare April 10, 2024 08:40
@MangoIV MangoIV merged commit dce1e8f into develop Apr 10, 2024
8 checks passed
@MangoIV MangoIV deleted the mangoiv/more-legalhold-tests branch April 10, 2024 09:36
fisx pushed a commit that referenced this pull request Apr 15, 2024
- [feat] port more legalhold tests to /integration
- [feat] introduce combinators for lazy Notifications in App 
---------

Co-authored-by: Marko Dimjašević <[email protected]>
Co-authored-by: Igor Ranieri <[email protected]>
Co-authored-by: Akshay Mankar <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ok-to-test Approved for running tests in CI, overrides not-ok-to-test if both labels exist
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants