fix_: sync fallback notification #5888
Conversation
Jenkins BuildsClick to see older builds (55)
|
Codecov ReportAttention: Patch coverage is
✅ All tests successful. No failed tests found. Additional details and impacted files@@ Coverage Diff @@
## develop #5888 +/- ##
===========================================
- Coverage 47.79% 47.45% -0.35%
===========================================
Files 834 833 -1
Lines 137749 137792 +43
===========================================
- Hits 65843 65394 -449
- Misses 64308 64624 +316
- Partials 7598 7774 +176
Flags with carried forward coverage won't be shown. Click here to find out more.
|
qfrank
force-pushed
the
fix/sync_fallback_notification2
branch
from
27c8b1c
to
824410e
Compare
There was a problem hiding this comment.
Changing my review because I just tested this branch and I no longer get any AC notification on both my apps.
Unless I need to modify my code in the Desktop app, but I doubt it since you just changed the way the AC notifs are triggered.
Here is an example, I synced the left app to the right one, so notifs of both:
thanks for reviewing! that's weird, you can see mobile QA test result, also the tests in |
qfrank
force-pushed
the
fix/sync_fallback_notification2
branch
from
364eef7
to
87494df
Compare
✔️ status-go/prs/android/PR-5888#4 🔹 ~2 min 40 sec 🔹 87494df 🔹 📦 android package |
igor-sirotin
changed the title
Ah! I know what happened. When I tested your branch, I still had an issue in my own branch. I'll test your branch again now that I fixed my own issue. |
| @@ -8,6 +8,9 @@ var ErrEnableInstallationAndPairInvalidID = errors.New("enable installation and | |||
|
|
|||
| type EnableInstallationAndPair struct { | |||
| InstallationID string `json:"installationId"` | |||
|
|
|||
| // used for testing | |||
| getInstallationID func() string | |||
There was a problem hiding this comment.
Although I like the simplicity, I don't think it's a good approach. Production code should not contain test things.
I think it should be feasible to unit-test saveDataAndPrepareResponse directly, by passing request with specific installationID, can you please try?
There was a problem hiding this comment.
If you find it not possible to unit-test, then we should go the proper mock way.
- Keep
EnableInstallationAndPairas before, no custom callback properties - Define
EnableInstallationAndPairInterfaceinterface withGetInstallationIDmethod - Implement
func (r *EnableInstallationAndPair) GetInstallationID() - Change
EnableInstallationAndPairsignature to takeEnableInstallationAndPairInterface. - Use
gomockto generate a mock in tests - Use the generated mocks in test
There was a problem hiding this comment.
Production code should not contain test things.
Make sense!
unit-test saveDataAndPrepareResponse directly
I don't think it's a good idea to write unit test for saveDataAndPrepareResponse, saveDataAndPrepareResponse is a bit complicated and composed, it's not that pure, unit test and integration test have different purpose and different usage scenarios
There was a problem hiding this comment.
If you find it not possible to unit-test, then we should go the proper mock way.
- Keep
EnableInstallationAndPairas before, no custom callback properties- Define
EnableInstallationAndPairInterfaceinterface withGetInstallationIDmethod- Implement
func (r *EnableInstallationAndPair) GetInstallationID()- Change
EnableInstallationAndPairsignature to takeEnableInstallationAndPairInterface.- Use
gomockto generate a mock in tests- Use the generated mocks in test
created interface InstallationIDProvider
There was a problem hiding this comment.
saveDataAndPrepareResponseis a bit complicated and composed
On the opposite, this is a very good reason for unit-testing 🙂
There was a problem hiding this comment.
I think it's easier for writing integration test as there already exist some cases.
There was a problem hiding this comment.
Well I didn't say it's gonna be easier 😄
But in the future we should try hard to write more unit tests than integration tests.
There was a problem hiding this comment.
But in the future we should try hard to write more unit tests than integration tests.
is it because the long time we need to wait for test result? if yes, I feel a little sad 😭 Maybe we should try other solution
✔️ status-go/prs/android/PR-5888#5 🔹 ~2 min 55 sec 🔹 bb2b1f9 🔹 📦 android package |
✔️ status-go/prs/android/PR-5888#9 🔹 ~2 min 0 sec 🔹 0056e30 🔹 📦 android package |
✔️ status-go/prs/tests/PR-5888#9 🔹 ~31 min 🔹 0056e30 🔹 📦 tests package |
qfrank
force-pushed
the
fix/sync_fallback_notification2
branch
3 times, most recently
from
12691fd
to
fcb02de
Compare
…ionV2 - Refactor EnableInstallationAndSync for better error handling and response merging - Add new EnableInstallationV2 method returning the installation - Update tests to check for installation in response - Deprecate old EnableInstallation method
…nID match - Add targetInstallationID parameter to SendPairInstallation function - Update protobuf SyncPairInstallation struct with TargetInstallationId field - Modify method calls across multiple test files to include new parameter - Update pairing.proto and pairing.pb.go with new field for local pairing
chore_: move InstallationIDProvider chore_: revert endpoints.go test_: check AC with resp chore_: rename ModifiedInstallationsTargetedToThisDevice test_: add InstallationIDProvider chore_: revert endpoints.go chore_: remove comment test_: add TestNewInstallationCreatedIsNotDeleted
qfrank
force-pushed
the
fix/sync_fallback_notification2
branch
from
fcb02de
to
8d38952
Compare
This PR already contains the changes made in PR 5869, because these two PRs have some intersections. To make things simple, let's just use this PR instead.
This PR mainly focuses on solving mobile issue 21260
We should not create/delete the notification if the target is not equal current installation id.
Relate mobile PR 21315
status: ready.