Paths

Table of Contentst

Differential D10422

[commtest] Add tests for Identity device list
ClosedPublic
Actions

Authored by bartek on Dec 21 2023, 2:15 AM.

Tags

None

Referenced Files

	F2170939: D10422.diff
	Tue, Jul 2, 4:28 PM

	Unknown Object (File)
	Sun, Jun 30, 7:29 PM

	Unknown Object (File)
	Sun, Jun 30, 6:27 AM

	Unknown Object (File)
	Sun, Jun 30, 4:46 AM

	Unknown Object (File)
	Sat, Jun 29, 10:56 PM

	Unknown Object (File)
	Sat, Jun 29, 2:03 PM

	Unknown Object (File)
	Tue, Jun 25, 3:24 PM

	Unknown Object (File)
	Tue, Jun 25, 12:04 PM

View All 48 Files

Subscribers

Details

Reviewers

michal
kamil
varun

Commits

rCOMM77d31fc5bb8a: [commtest] Add tests for Identity device list

Summary

Added integration tests for device list. I ended up with a single integration test that tests the following:

Adding devices during register
Adding devices during login
Removing devices on logout
Reordering devices so mobile is the primary device when possible
Getting device list updates for another user and verifying their order

Depends on D10421

Test Plan

CommTest, CI

Diff Detail

Repository

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

bartek created this revision.Dec 21 2023, 2:15 AM

bartek held this revision as a draft.

Herald added subscribers: tomek, ashoat. · View Herald TranscriptDec 21 2023, 2:15 AM

Harbormaster failed remote builds in B25263: Diff 34896!Dec 21 2023, 2:32 AM

Rebase on fix

bartek added a parent revision: D10423: [identity] Fix log_out_user DDB expression error.Dec 21 2023, 3:14 AM

bartek published this revision for review.Dec 21 2023, 3:48 AM

Harbormaster completed remote builds in B25269: Diff 34902.Dec 21 2023, 4:39 AM

bartek mentioned this in rCOMM84437bdc18b4: [identity] Fix log_out_user DDB expression error.Dec 21 2023, 6:03 AM

Looks good, but probably it's better for @varun to have a final look

services/commtest/tests/identity_device_list_tests.rs
25–26	to make it a bit more readable I would probably do something like this
30	just wondering, are you sure this is not interfering with tests that we use the same keys (`DEFAULT_CLIENT_KEYS`) during registration? (here for the viewer and later for Android device)
42	probably can be removed?
72	I would use here and in line 64 `android` to make it obvious we are using the same user but different devices, or maybe put both generated `username` and `user_id` in meaningful variables, but it is just a test so if you think it's okay this also looks fine

bartek added inline comments.Jan 5 2024, 2:22 AM

services/commtest/tests/identity_device_list_tests.rs
25–26	good idea
30	No, keys need to be unique per-user. Nothing in Identity Service treats them "globally" (e.g. sync between users or sth)
42	yep
72	Makes sense. The only reason I used `ios.user_id` was to avoid cloning 😅

varun accepted this revision.Jan 7 2024, 8:11 PM

varun added inline comments.

services/commtest/tests/identity_device_list_tests.rs
51	Log in (applies to other comments too)

This revision is now accepted and ready to land.Jan 7 2024, 8:11 PM

bartek added a parent revision: D10404: [identity] Reorder device list when updated.Jan 9 2024, 5:50 AM

Address review feedback

Harbormaster completed remote builds in B25726: Diff 35575.Jan 12 2024, 7:47 AM

Rebase before landing

Harbormaster completed remote builds in B25764: Diff 35616.Jan 15 2024, 6:50 AM

Closed by commit rCOMM77d31fc5bb8a: [commtest] Add tests for Identity device list (authored by bartek). · Explain WhyJan 15 2024, 6:53 AM

This revision was automatically updated to reflect the committed changes.

bartek added a commit: rCOMM77d31fc5bb8a: [commtest] Add tests for Identity device list.

Revision Contents
Changeset List

Path

Size

services/

commtest/

run-tests-ci.sh

1 line

tests/

identity_device_list_tests.rs

127 lines

Diff 34902

services/commtest/run-tests-ci.sh

Loading...

services/commtest/tests/identity_device_list_tests.rs

Loading...