Set fleet to shards.test #18115
Merged
Set fleet to shards.test #18115
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Jenkins BuildsClick to see older builds (68)
|
|
@cammellos status.prod fleet is set by default on new account and no shards.test in fleets
|
cammellos
force-pushed
the
chore/set-default-fleet-to-shard-test
branch
3 times, most recently
from
1ffd2cd to
a21a568
Compare
12% of end-end tests have passedFailed tests (39)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestActivityMultipleDevicePRTwo:
Class TestCommunityMultipleDeviceMerged:
Class TestActivityMultipleDevicePR:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestCommunityOneDeviceMerged:
Expected to fail tests (4)Click to expandClass TestCommunityOneDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestGroupChatMultipleDeviceMergedNewUI:
Passed tests (6)Click to expandClass TestCommunityOneDeviceMerged:
Class TestDeepLinksOneDevice:
|
ilmotta
reviewed
Dec 8, 2023
| [_ chat-id] | ||
| {:json-rpc/call [{:method "wakuext_fetchMessages" | ||
| :params [{:id chat-id}] | ||
| :on-success #() |
There was a problem hiding this comment.
Some time ago I changed on-success to be optional, so you can remove this placeholder function.
src/status_im2/subs/profile.cljs
Outdated
| (fn [{:keys [webview-debug]}] | ||
| webview-debug)) | ||
|
|
||
| (re-frame/reg-sub |
There was a problem hiding this comment.
FYI: We have configured zprint to allow the sub ID to be on the same line as reg-sub, as well as for reg-event-fx and reg-fx. However, it's not something we are consistently following in the code yet. Being on the same line is very helpful for conducting project-wide searches, line by line, to, for example, find a subscription containing word X. I miss this a few times every week. Granted, we can grep with the surrounding context, but it's one more thing to remember.
| {:icon :i/download | ||
| :right-icon :i/chevron-right | ||
| :accessibility-label :chat-fetch-messages | ||
| :on-press (partial fetch-messages! chat-id) |
There was a problem hiding this comment.
Normally in the repo we don't use exclamation mark in functions with side-effects. There's also no guideline for this. In my experience it's a convention that's too hard for a large team to follow and no linter can reliably help in Clojure. There are way too many functions with side-effects in frontend code too, so it can be a chore to stay consistent with the convention.
But here you had to use to avoid the naming conflict right? But inlining on-press is clear and avoid the indirection too:
(defn fetch-messages
[chat-id]
{:icon :i/download
:right-icon :i/chevron-right
:accessibility-label :chat-fetch-messages
:on-press (fn []
(rf/dispatch [:hide-bottom-sheet])
(rf/dispatch [:chat/fetch-messages chat-id]))
:label (i18n/label :t/fetch-messages)})
src/status_im/keycard/recovery.cljs
Outdated
| {})))) | ||
| ;; NOTE: Disabling as keycard not functional and this needs to be moved to | ||
| ;; status-go | ||
| #_(multiaccounts.create/on-multiaccount-created |
There was a problem hiding this comment.
To me it would be best to have an issue, delete all the zombie code and add to the issue's description a link to the revision with the original source (for reference). I think it's convenient to comment code out, but this makes the code confusing and zombie code tends to live for a long time (maybe will not the case here?)
There was a problem hiding this comment.
oh sorry, missed the comment, I can remove it no problem
src/status_im/fleet/core.cljs
Outdated
| [status-im2.constants :as constants] | ||
| [utils.i18n :as i18n] | ||
| [utils.re-frame :as rf])) | ||
| [utils.re-frame :as rf] |
There was a problem hiding this comment.
It would be nice to see some of the old code you are touching moving to new namespaces in status-im2 and following guidelines. Not in this PR of course, but one or two follow-up ones could make a big difference. When we touch old code we have more context to make safer changes, so it's a good opportunity :)
src/status_im/fleet/core.cljs
Outdated
| [utils.re-frame :as rf] | ||
| [utils.transforms :as transforms])) | ||
|
|
||
| (def default-fleets (transforms/json->js (native-module/fleets))) |
There was a problem hiding this comment.
It helps during refactoring to know when a var is private or not as we don't need to search it outside the file to be sure it's safer to move it, rename it or refactor it. default-fleets and default-fleet can be private.
src/status_im/fleet/core.cljs
Outdated
| [utils.transforms :as transforms])) | ||
|
|
||
| (def default-fleets (transforms/json->js (native-module/fleets))) | ||
| (def default-fleet (.-defaultFleet default-fleets)) |
There was a problem hiding this comment.
Apparently, we don't need to write interop code here. Suggestion:
(def ^:private default-fleets (transforms/json->clj (native-module/fleets)))
(def ^:private default-fleet (:defaultFleet default-fleets))
(def fleets (->> default-fleets :fleets keys (map keyword)))There was a problem hiding this comment.
I was trying to avoid expensive conversions to clojure, since this code is run at startup of the app, and that's where we want to be lean, probably does not matter so much, but it's a place where potentially we could be death by a thousand of paper cuts, but it's probably ok in this case
There was a problem hiding this comment.
@cammellos, a tradeoff you can make here to remove the cost at startup completely is to compute the fleets in a subscription or just make all three vars functions and call (fleets/fleets) in fleet-settings.
Perhaps this is the best we should do because most users won't navigate or care about the fleet settings, and it's fine there to compute from scratch, the perf hit should be minimal and not impact other parts of the app.
ISSUE 1: CRs are not receivedLogs for sender: Status-debug-logs.zip Also I noticed that display name hasn't been resolved at all
ISSUE 2: Error when open advanced > Fleets
|
cammellos
force-pushed
the
chore/set-default-fleet-to-shard-test
branch
3 times, most recently
from
44fca8f to
c967d0d
Compare
|
@ilmotta I think I have addressed the feedback, would you mind having another look please? |
ilmotta
approved these changes
Dec 8, 2023
There was a problem hiding this comment.
@ilmotta I think I have addressed the feedback, would you mind having another look please?
thank you!
PR looks good @cammellos, thank you.
The only comment I didn't see addressed so that I can sleep like a baby was https://github.com/status-im/status-mobile/pull/18115/files#r1419797218 😄
cammellos
force-pushed
the
chore/set-default-fleet-to-shard-test
branch
from
c967d0d to
4585cd6
Compare
|
@cammellos installed builds with c967d0d :
Receiver logs: logs (71).zip |
0% of end-end tests have passedFailed tests (39)Click to expandClass TestCommunityMultipleDeviceMerged:
Class TestActivityMultipleDevicePRTwo:
Class TestActivityMultipleDevicePR:
Class TestCommunityOneDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestActivityCenterContactRequestMultipleDevicePR:
|
cammellos
force-pushed
the
chore/set-default-fleet-to-shard-test
branch
2 times, most recently
from
97517a9 to
3a6baa5
Compare
62% of end-end tests have passedFailed tests (12)Click to expandClass TestCommunityMultipleDeviceMergedTwo:
Class TestCommunityOneDeviceMerged:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Expected to fail tests (3)Click to expandClass TestCommunityMultipleDeviceMergedTwo:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityMultipleDeviceMerged:
Passed tests (24)Click to expandClass TestCommunityMultipleDeviceMergedTwo:
Class TestActivityMultipleDevicePRTwo:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestActivityMultipleDevicePR:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestCommunityMultipleDeviceMerged:
|
41% of end-end tests have passedFailed tests (23)Click to expandClass TestCommunityOneDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestActivityMultipleDevicePR:
Class TestCommunityMultipleDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Expected to fail tests (6)Click to expandClass TestCommunityOneDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Passed tests (20)Click to expandClass TestCommunityOneDeviceMerged:
Class TestActivityMultipleDevicePRTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestDeepLinksOneDevice:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
|
|
Message reliability issues are the reasons for failed e2e. 1) Reaction was not received in 2 minsSender: session step(08:47 in tests) Receiver: session step(10:33, still no reaction) 2) All group tests failed due to long accept of CR.Within 90 sec after clicking on Accept the user (member_1) still hasn't been added as a contact of admin. Member_1 (accept the request): session step at 07:59 Admin (receiver): session step from 8:00 to 09:40 - member_1 is not added to contacts, so there is no message of accepting CR from member_1 3) Most community tests failed because the joining the open community request was stuck in "Pending" > 60 secCommunity admin: session step (08:46, after this step request should be automatically approved) Member: session step(sending Request to join at 09:10, to 10:11 still not joined) |
cammellos
force-pushed
the
chore/set-default-fleet-to-shard-test
branch
from
3a6baa5 to
e185a75
Compare
78% of end-end tests have passedFailed tests (4)Click to expandClass TestCommunityMultipleDeviceMergedTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityOneDeviceMerged:
Class TestCommunityMultipleDeviceMerged:
Expected to fail tests (7)Click to expandClass TestCommunityOneDeviceMerged:
Class TestCommunityMultipleDeviceMerged:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestGroupChatMultipleDeviceMergedNewUI:
Passed tests (38)Click to expandClass TestActivityCenterContactRequestMultipleDevicePR:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestActivityMultipleDevicePR:
Class TestCommunityOneDeviceMerged:
Class TestDeepLinksOneDevice:
Class TestCommunityMultipleDeviceMerged:
Class TestActivityMultipleDevicePRTwo:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestGroupChatMultipleDeviceMergedNewUI:
|
|
I couldn't receive any messages in 1-1 chat and in the community that should be fetched from online for > 10 mins
received only part of the message history from community that was marked as "sending" and was resent after receiver got back online; UPDATE: eventually messages were fetched > 15 mins and after several attempts of re-login Logs attached after messages were fetched. Sender: Status-debug-logs.zip |
cammellos
force-pushed
the
chore/set-default-fleet-to-shard-test
branch
2 times, most recently
from
fa53f1d to
ebbe5ac
Compare
|
been testing out this branch and seems okay for me. I can chat 1-1, syncs with desktop etc 👍 In general looks good. Will update if I happen to find any issues |
J-Son89
approved these changes
Dec 18, 2023
|
@churik how were the e2e in the last run? |
|
let me update status-go actually |
cammellos
force-pushed
the
chore/set-default-fleet-to-shard-test
branch
from
ebbe5ac to
c275e69
Compare
|
@cammellos Let me re-run tests one more time. |
40% of end-end tests have passedFailed tests (26)Click to expandClass TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityMultipleDeviceMerged:
Class TestActivityMultipleDevicePR:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestActivityMultipleDevicePRTwo:
Class TestCommunityOneDeviceMerged:
Expected to fail tests (3)Click to expandClass TestCommunityOneDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestGroupChatMultipleDeviceMergedNewUI:
Passed tests (19)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestDeepLinksOneDevice:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestCommunityOneDeviceMerged:
|
|
Most of the tests were failed due to the impossibility of fetching community data while tapping on the community link: Steps:
Logs (User1): TestCommunityMultipleDeviceMerged_geth0 (4).log Logs (User2): TestCommunityMultipleDeviceMerged_geth1 (4).log This is reproducible manually too, using + > "Create open community" button) - maybe the method needs to be adjusted on status-go side (not sure)? |
cammellos
force-pushed
the
chore/set-default-fleet-to-shard-test
branch
3 times, most recently
from
6725faa to
c6c4b9e
Compare
|
after last changes cannot receive a contact request (only ONCE, could not reproduce manually anymore) Leaving here comment to preserve the history Receiver: |
|
Found an issue (Android only, tested on Pixel 7A, Android 13) - messages are not resent to the community.
On IOS messages are resent ~ for 1 min, on Android they are never re-sent. Logs from Android: Video: FILE.2023-12-19.19.39.02.mp4So basically all these messages are stuck in "Sending" state. |
69% of end-end tests have passedFailed tests (10)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityOneDeviceMerged:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestActivityMultipleDevicePR:
Expected to fail tests (5)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestCommunityOneDeviceMerged:
Passed tests (33)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestCommunityOneDeviceMerged:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestActivityMultipleDevicePRTwo:
Class TestDeepLinksOneDevice:
Class TestCommunityMultipleDeviceMerged:
|
cammellos
force-pushed
the
chore/set-default-fleet-to-shard-test
branch
from
c6c4b9e to
dc083d2
Compare
69% of end-end tests have passedFailed tests (10)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestActivityMultipleDevicePR:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityOneDeviceMerged:
Class TestCommunityMultipleDeviceMergedTwo:
Expected to fail tests (5)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestCommunityOneDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestGroupChatMultipleDeviceMergedNewUI:
Passed tests (33)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestActivityMultipleDevicePRTwo:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestActivityMultipleDevicePR:
Class TestDeepLinksOneDevice:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityMultipleDeviceMerged:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestCommunityOneDeviceMerged:
|
81% of end-end tests have passedFailed tests (4)Click to expandClass TestCommunityOneDeviceMerged:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestActivityMultipleDevicePRTwo:
Expected to fail tests (5)Click to expandClass TestCommunityOneDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Passed tests (39)Click to expandClass TestCommunityOneDeviceMerged:
Class TestCommunityMultipleDeviceMerged:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestActivityMultipleDevicePR:
Class TestDeepLinksOneDevice:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
|
cammellos
force-pushed
the
chore/set-default-fleet-to-shard-test
branch
from
eb70e03 to
41b7077
Compare
This PR does a few things: 1) Add fetch messages implementation on any kind of chat. It's behind a toggle (on by default) as design is still unsure whether we want it, but it's very useful for debugging. 2) Allow setting light client from mobile It also partially remove node config management from the clojure part, as it's better if that's not explicitly managed by clients. Some parts are still relying on it but they are not functional (keycard), while others are still using it and will need to be updated eventually (syncing), in order to get rid completely of node config. Sets fleet to shards.test status-im/status-go@90c31af...1adcf02
cammellos
force-pushed
the
chore/set-default-fleet-to-shard-test
branch
from
41b7077 to
32cfd21
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Set fleet to shards.test and remove dependency on status-mobile for fleet definition (there's still some work to be done on syncing to completely remove all the node config/fleet from status-mobile and have everything driven by status-go)
Behavior
New accounts should have
shards.testfleet set by defaultMigrated accounts should have
shards.testfleet set by default if they never set one beforeincludes #18007 as related changes