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

integration: Fix testAddingUserNonFullyConnectedFederation and testNotificationsForOfflineBackends #3529

Merged
merged 4 commits into from
Aug 23, 2023
Merged

integration: Fix testAddingUserNonFullyConnectedFederation and testNotificationsForOfflineBackends #3529

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
863b383
integration: Fix testAddingUserNonFullyConnectedFederation
akshaymankar Aug 23, 2023
25437f4
integration: Don't allow adding users to conv when one of the paritic…
akshaymankar Aug 23, 2023
09558b7
integration: Add retries to get around problem of federation domain s…
akshaymankar Aug 23, 2023
9685456
Typo
akshaymankar Aug 23, 2023
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
8 changes: 8 additions & 0 deletions integration/test/API/BrigInternal.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,14 @@ deleteFedConn' owndom dom = do
req <- rawBaseRequest owndom Brig Unversioned ("/i/federation/remotes/" <> dom)
submit "DELETE" req

deleteAllFedConns :: (HasCallStack, MakesValue dom) => dom -> App ()
deleteAllFedConns dom = do
readFedConns dom >>= \resp ->
resp.json %. "remotes"
& asList
>>= traverse (\v -> v %. "domain" & asString)
>>= mapM_ (deleteFedConn dom)

registerOAuthClient :: (HasCallStack, MakesValue user, MakesValue name, MakesValue url) => user -> name -> url -> App Response
registerOAuthClient user name url = do
req <- baseRequest user Brig Unversioned "i/oauth/clients"
Expand Down
37 changes: 26 additions & 11 deletions integration/test/Test/Conversation.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
{-# OPTIONS_GHC -Wno-ambiguous-fields #-}
{-# OPTIONS_GHC -Wno-incomplete-uni-patterns #-}

module Test.Conversation where

Expand Down Expand Up @@ -410,19 +411,33 @@ testAddUnreachable = do
testAddingUserNonFullyConnectedFederation :: HasCallStack => App ()
testAddingUserNonFullyConnectedFederation = do
let overrides =
def {dbBrig = setField "optSettings.setFederationStrategy" "allowAll"}
<> fullSearchWithAll
startDynamicBackends [overrides] $ \domains -> do
own <- make OwnDomain & asString
other <- make OtherDomain & asString
[alice, alex, bob, charlie] <-
createAndConnectUsers $ [own, own, other] <> domains

let newConv = defProteus {qualifiedUsers = [alex]}
def
{ dbBrig =
setField "optSettings.setFederationStrategy" "allowDynamic"
>=> removeField "optSettings.setFederationDomainConfigs"
}
startDynamicBackends [overrides] $ \[dynBackend] -> do
own <- asString OwnDomain
other <- asString OtherDomain

-- Ensure that dynamic backend only federates with own domain, but not other
-- domain.
--
-- FUTUREWORK: deleteAllFedConns at the time of acquiring a backend, so
-- tests don't affect each other.
deleteAllFedConns dynBackend
void $ createFedConn dynBackend (FedConn own "full_search")
Comment on lines +426 to +429
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you please explain how this dance of deleting and then immediately making a connection works?

Copy link
Member Author

Choose a reason for hiding this comment

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

If you have two tests: one of them connects dynBackend1 with all backends and second one only connects dynBackend1 with OwnDomain. if the first test runs first, the second test will always fail. So, the second test must always delete all the connections dynBackend1 has and then only create a connection with OwnDomain to ensure that it is not affected by the first test.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see.

Wasn't the plan to provide more isolation between test executions? This dance within the test is very fragile, it incurs what I hope can be called a needless brain load, and is not a general solution.

Is there something we can do in the (dynamic) backend setup that would make them isolated from each other (even if we go with e.g. randomly assigning identifiers (names) and such, which should be good enough if they are very unlikely to collide)?

Copy link
Member Author

@akshaymankar akshaymankar Aug 23, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, that's why I added the FUTUREWORK. While acquiring an environment we could simply truncate the table which keeps this information around.


alice <- randomUser own def
bob <- randomUser other def
charlie <- randomUser dynBackend def
mapM_ (connectUsers alice) [bob, charlie]

let newConv = defProteus {qualifiedUsers = []}
conv <- postConversation alice newConv >>= getJSON 201

bobId <- bob %. "qualified_id"
charlieId <- charlie %. "qualified_id"
bindResponse (addMembers alex conv [bobId, charlieId]) $ \resp -> do
bindResponse (addMembers alice conv [bobId, charlieId]) $ \resp -> do
resp.status `shouldMatchInt` 409
resp.json %. "non_federating_backends" `shouldMatchSet` (other : domains)
resp.json %. "non_federating_backends" `shouldMatchSet` [other, dynBackend]