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.

Sign up for GitHub

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

Deflake invitation too many pending #3092

Merged
merged 1 commit into from
Feb 16, 2023
Merged

Deflake invitation too many pending #3092

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 9 additions & 6 deletions services/brig/test/integration/API/Team.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,7 @@ tests conf m n b c g aws = do
test m "post /teams/:tid/invitations - email lookup nginz" $ testInvitationEmailLookupNginz b n,
test m "post /teams/:tid/invitations - email lookup register" $ testInvitationEmailLookupRegister b,
test m "post /teams/:tid/invitations - 403 no permission" $ testInvitationNoPermission b,
test m "post /teams/:tid/invitations - 403 too many pending" $ testInvitationTooManyPending b tl,
test m "post /teams/:tid/invitations - 403 too many pending" $ testInvitationTooManyPending conf b tl,
test m "post /teams/:tid/invitations - roles" $ testInvitationRoles b g,
test m "post /register - 201 accepted" $ testInvitationEmailAccepted b g,
test m "post /register - 201 accepted (with domain blocking customer extension)" $ testInvitationEmailAcceptedInBlockedDomain conf b g,
Expand Down Expand Up @@ -354,14 +354,17 @@ headInvitationByEmail service email expectedCode =
Bilge.head (service . path "/teams/invitations/by-email" . contentJson . queryItem "email" (toByteString' email))
!!! const expectedCode === statusCode

testInvitationTooManyPending :: Brig -> TeamSizeLimit -> Http ()
testInvitationTooManyPending brig (TeamSizeLimit limit) = do
testInvitationTooManyPending :: Opt.Opts -> Brig -> TeamSizeLimit -> Http ()
testInvitationTooManyPending opts brig (TeamSizeLimit limit) = do
(inviter, tid) <- createUserWithTeam brig
emails <- replicateConcurrently (fromIntegral limit) randomEmail
pooledForConcurrentlyN_ 16 emails $ postInvitation brig tid inviter . stdInvitationRequest
email <- randomEmail
-- TODO: If this test takes longer to run than `team-invitation-timeout`, then some of the
-- invitations have likely expired already and this test will actually _fail_
-- If this test takes longer to run than `team-invitation-timeout`, then some of the
-- invitations have likely expired already and this test will actually _fail_
-- therefore we increase the timeout from default 10 to 300 seconds
let longerTimeout = opts {Opt.optSettings = (Opt.optSettings opts) {Opt.setTeamInvitationTimeout = 300}}
withSettingsOverrides longerTimeout $ do
forM_ emails $ postInvitation brig tid inviter . stdInvitationRequest
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know what's the setMaxTeamSize value in options passed to the test, but if it's big, could it be that doing this sequentially takes a bit too long? I was planning to make a very little team size limit, along these lines:

  let smallTeam =
        opts
          { Opt.optSettings =
              (Opt.optSettings opts)
                { Opt.setTeamInvitationTimeout = 300,
                  Opt.setMaxTeamSize = 5
                }
          }
  withSettingsOverrides smallTeam $ do

Copy link
Member Author

Choose a reason for hiding this comment

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

I was thinking since tests already run in parallel, we may not gain that much by spawning even more threads, as these are just simple http requests that should be at milisecond response time level in theory (when there isn't anything else ongoing and enough CPU/memory available). Right now the team size is 32 that is inside the hack/helm_vars/... override. Setting maxTeamSize to something smaller is a good idea though, your suggestion makes sense, feel free to adjust this test to this in a separate PR (I see you already merged this one as-is).

postInvitation brig tid inviter (stdInvitationRequest email) !!! do
const 403 === statusCode
const (Just "too-many-team-invitations") === fmap Error.label . responseJsonMaybe
Expand Down