Add tests for UsersManager #1149

fflorent · 2024-08-05T16:21:23Z

Context

HomeDBManager lacks of direct tests, which makes hard to make rework or refactorations.

Proposed solution

Specifically here, I introduce tests which call exposed UsersManager methods directly and check their result.

Also:

I removed updateUserName which seems to me useless (updateUser does the same work)
Taking a look at the getUserByLogin methods, it appears that Typescirpt infers it returns a Promise<User|null> while in no case it may resolve a nullish value, therefore I have forced to return a Promise<User> and have changed the call sites to reflect the change.

Related issues

I make this change for then working on #870

Has this been tested?

👍 yes, I added tests to the test suite
💭 no, because this PR is a draft and still needs work
🙅 no, because this is not relevant here
🙋 no, because I need help

fflorent

Comments after having taken a look at the PR with @hexaltation

test/gen-server/lib/homedb/UsersManager.ts

app/gen-server/lib/homedb/UsersManager.ts

hexaltation

LGTM

app/gen-server/lib/homedb/UsersManager.ts

app/server/companion.ts

berhalak · 2024-08-15T15:12:03Z

test/gen-server/lib/homedb/UsersManager.ts

+import { updateDb } from 'app/server/lib/dbUtils';
+
+const username = process.env.USER || "nobody";
+const tmpDir = path.join(tmpdir(), `grist_test_${username}_userendpoint`);


Should we randomize it more? Runing this test twice will reuse the same folder?
https://dev.to/omardulaimi/how-to-create-unique-temporary-directories-in-nodejs-1n69

I would say it is not necessary.
The point here is:

to have a temporary path, so it does not pollute the user filesystem;

to have a predictable path, so you can take a look after the tests run at the assets (here the databases) if something surprising happened;

to have this folder cleared before each run thanks to the call of prepareFilesystemDirectoryForTests (called here), so the test run is not affected by previous ones.

What do you think? Does it make sense to you?

I'm just worry about running this tests twice (simultaneously) on the same machine, most of our tests (if not all of them) can be run simultaneously without a problem.

I see!

FWIW, I took inspiration of what those other tests suit do:

Webhook-Proxies (tmpDir declaration and use of prepareFilesystemDirectoryForTests);

DocsApi (tmpDir declaration and use of prepareFilesystemDirectoryForTests);

That being said, I can think of something that would get the best of the two worlds.

@berhalak I propose something to address your concerns, and still have the ability to disable the tests cleanup for debugging: 4031257

What do you think?

test/gen-server/lib/homedb/UsersManager.ts

berhalak · 2024-08-15T15:13:13Z

test/gen-server/lib/homedb/UsersManager.ts

+  this.timeout(30000);
+
+  describe('static method', function () {
+    function* makeIdxIterator() {


Can you move helper methods to the end of the file? (And even outside of the describe funcition)

I prefer having the helper functions in the most limited scope, so it is clear where it is meant to be used, and for what purpose.

That being said, if you say your convention is to rather put all the helper at the end, I would agree to follow it.

Ok, I don't have a strong opinion here. I just prefer to have them hidden, or not to have them at all :).

berhalak · 2024-08-15T15:16:25Z

test/gen-server/lib/homedb/UsersManager.ts

+      }
+    }
+
+    function makeUsers(nbUsers: number, idxIt = makeIdxIterator()) {


Can you use here the range function from lodash? I think it will look much more simpler, this one is hard to follow.

I need to keep track of the increment for users' id across makeUsers call. When I call makeUsers let's say this way:

const idxIt = makeIdxIterator(); const groupsUsersMap = { 'editors': makeUsers(2, idxIt), 'owners': makeUsers(1, idxIt), };

groupsUsersMap equals to the following object, whose users' id are unique:

{ 'editors': [{id: 0, name: 'user 0'}, {id: 1, name: 'user 1'}], 'owners': [{id: 2, name: 'user 2'}] }

I don't know how to make a similar thing with lodash.range().

An alternative I can do, but I find it less elegant, would consist in passing the start index each time:

let startIdx = 0; const groupsUsersMap = {}; groupsUsersMap['editors'] = makeUsers(2, startIdx); startIdx += 2; groupsUsersMap['owners'] = makeUsers(1, startIdx); startIdx += 1;

berhalak · 2024-08-15T15:17:08Z

test/gen-server/lib/homedb/UsersManager.ts

+      });
+    }
+
+    function makeUsersList(usersListLength: number, nbUsersByGroups: number) {


What does nb stands for?

number of. I am commenting the function, with the hope to make things clearer

fflorent · 2024-08-16T14:35:03Z

@berhalak I think of something: my test assume that they run using Sqlite.

Several questions come to my mind:

Can running these tests with Postgresql be worth?
If so: I don't see tests running with Postgresql on the CI, I can take a look to see how hard it is to support this database.
If not: I propose to skip these tests when TYPEORM_TYPE is set to postgresql

To the first question, my answer would be no, the UsersManager uses a lot of TypeORM methods, and does not seem to make SQL query directly (but I may be a bit biased by my lazyness).

…sh>)

Mostly styling issues

@berhalak

To address @berhalak comment: > I'm just worry about running this tests twice (simultaneously) on the same machine, most of our tests (if not all of them) can be run simultaneously without a problem.

fflorent · 2024-08-19T14:24:22Z

test/gen-server/seed.ts

  }
 }

+export function dereferenceConnection(name: string) {
+  // There seem to be no official way to delete connections.
+  // Also we should probably get rid of the use of connectionManager, which is deprecated


I am tempted to do that right now, but I fear that the PR becomes huge, especially to review it.

The new way consist in using DataSource (which is the new name for the Connection class, renamed because it is confusing), and instantiate using the constructor in place of createConnection.

We won't have any name collision, as only the ConnectionManager use it for other purposes than identifying the DataSource in error messages.

berhalak

Thanks @fflorent!

berhalak · 2024-08-23T08:23:55Z

test/gen-server/seed.ts

-export async function removeConnection() {
-  if (getConnectionManager().connections.length > 0) {
-    if (getConnectionManager().connections.length > 1) {
+export async function removeConnection(name?: string) {


The name parameter doesn't seem to be used here.

That's true, thanks!

berhalak · 2024-08-23T09:52:47Z

Can running these tests with Postgresql be worth?

We run some tests with Postgresql in our internal CI, but since it is not part of the grist-core yet, and those tests are rather unit tests, I think it is fine to rely on typeorm abstraction.

paulfitz · 2024-08-23T16:24:57Z

I'm seeing failures in tests after UsersManager in a derived version of Grist (the one for our SaaS). Haven't looked into why yet, and may be specific to us.

paulfitz · 2024-08-23T16:28:51Z

test/gen-server/seed.ts

+  if (!connectionMap.has(name)) {
+    throw new Error('connection with this name not found: ' + name);
+  }
+  connectionMap.delete(name);


Is it possible this results in the change of behavior we see in our tests @fflorent? This is a pretty murky corner of TypeORM.

I feel more like the issue is due to this helper:
https://github.com/gristlabs/grist-core/pull/1149/files#diff-3d1ec6a021479fb3895f89f5c855b14f664e45c31e979082da277538b906cc5cR264-R282

Which has been introduced to test functions related to special ids, but I feel like these two tests bring more pain than are actually helpful:

The test that check the getters related to the special IDs without initialization fail

The test of initializeSpecialIds

I also tried to see whether removing the Connection related methods in favor of using DataSource would help, but with no luck (see #1166, which I may continue to work on but rather for the satisfaction of stop using deprecated entities and functions).

So I won't mind removing these tests, especially if it solves your failures, I agree that it deals with strange behaviors of TypeORM.

It is awkward getting this sorted out, so I'm going to land and that'll force us to sort it out internally. Thanks for the suggestions.

fflorent · 2024-09-05T20:37:47Z

Thank you @paulfitz!

Context HomeDBManager lacks of direct tests, which makes hard to make rework or refactorations. Proposed solution Specifically here, I introduce tests which call exposed UsersManager methods directly and check their result. Also: I removed updateUserName which seems to me useless (updateUser does the same work) Taking a look at the getUserByLogin methods, it appears that Typescirpt infers it returns a Promise<User|null> while in no case it may resolve a nullish value, therefore I have forced to return a Promise<User> and have changed the call sites to reflect the change. Related issues I make this change for then working on gristlabs#870

fflorent force-pushed the users-management-tests branch 4 times, most recently from b65d331 to c5a693e Compare August 5, 2024 19:40

fflorent mentioned this pull request Aug 5, 2024

Enabling BiDi mode - fixing test failures #1152

Merged

3 tasks

fflorent force-pushed the users-management-tests branch 3 times, most recently from 2cf3fdb to abdfccc Compare August 6, 2024 09:57

fflorent commented Aug 6, 2024

View reviewed changes

fflorent force-pushed the users-management-tests branch 3 times, most recently from ca8492a to 7d8b326 Compare August 7, 2024 10:17

fflorent requested a review from hexaltation August 12, 2024 09:19

hexaltation approved these changes Aug 14, 2024

View reviewed changes

fflorent self-assigned this Aug 14, 2024

berhalak self-requested a review August 15, 2024 14:35

berhalak requested changes Aug 15, 2024

View reviewed changes

app/gen-server/lib/homedb/UsersManager.ts Outdated Show resolved Hide resolved

app/server/companion.ts Outdated Show resolved Hide resolved

berhalak reviewed Aug 15, 2024

View reviewed changes

fflorent requested a review from berhalak August 16, 2024 13:19

fflorent force-pushed the users-management-tests branch 3 times, most recently from 42eee85 to 2a2ccf1 Compare August 19, 2024 13:32

fflorent added 7 commits August 19, 2024 15:47

Start of UsersManager tests

314b03e

Test ensureExternalUser()

2f05943

new progress on UsersManager unit tests

48f5d33

test deleteUser()

a6bd18a

Tests for getLoginWithRetry

d2b185c

initializeSpecialIds and completeProfiles()

71e0599

Fix tests for initializeSpecialIds()

f9148fe

fflorent added 14 commits August 19, 2024 15:47

Replace uuid with unique local parts

4c0b087

make getUserByLogin return Promise<User> (cannot return Promise<nulli…

ca18de7

…sh>)

Also force getUserByLoginWithRetry to return a Promise<User>

ad499e3

Self-remarks

4ecb24b

Fix test failure for getUserByLoginWithRetry

d7af3af

Apply suggestions from code review

cd68866

Mostly styling issues

More remarks taken into account

705a34f

@berhalak remarks and rework of the static methods tests

a5fd314

Rename generator helper function to be more accurate

6476676

Make a temp dir with random suffix the tests

8ebe474

To address @berhalak comment: > I'm just worry about running this tests twice (simultaneously) on the same machine, most of our tests (if not all of them) can be run simultaneously without a problem.

Sort imports

894c55e

Skip tests when using Postgresql

55d10b3

Introduce dereferenceConnection for TypeORM to be used in tests

e1f97ee

Fix eslint errors after rebase

eaaff1b

fflorent force-pushed the users-management-tests branch from 2a2ccf1 to eaaff1b Compare August 19, 2024 13:49

fflorent commented Aug 19, 2024

View reviewed changes

berhalak approved these changes Aug 23, 2024

View reviewed changes

@berhalak remark: name is not used in removeConnection

a77e12a

fflorent requested a review from berhalak August 23, 2024 11:10

paulfitz reviewed Aug 23, 2024

View reviewed changes

paulfitz merged commit 16ebc32 into gristlabs:main Sep 5, 2024
11 checks passed

fflorent deleted the users-management-tests branch September 5, 2024 20:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add tests for UsersManager #1149

Add tests for UsersManager #1149

fflorent commented Aug 5, 2024

fflorent left a comment

hexaltation left a comment

berhalak Aug 15, 2024

fflorent Aug 16, 2024 •

edited

Loading

berhalak Aug 16, 2024

fflorent Aug 16, 2024

fflorent Aug 16, 2024

berhalak Aug 15, 2024

fflorent Aug 16, 2024

berhalak Aug 16, 2024

berhalak Aug 15, 2024

fflorent Aug 16, 2024

berhalak Aug 15, 2024

fflorent Aug 16, 2024

fflorent commented Aug 16, 2024

fflorent Aug 19, 2024

berhalak left a comment

berhalak Aug 23, 2024

fflorent Aug 23, 2024

berhalak commented Aug 23, 2024

paulfitz commented Aug 23, 2024

paulfitz Aug 23, 2024

fflorent Aug 23, 2024

paulfitz Sep 5, 2024

fflorent commented Sep 5, 2024

Add tests for UsersManager #1149

Add tests for UsersManager #1149

Conversation

fflorent commented Aug 5, 2024

Context

Proposed solution

Related issues

Has this been tested?

fflorent left a comment

Choose a reason for hiding this comment

hexaltation left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fflorent Aug 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fflorent commented Aug 16, 2024

Choose a reason for hiding this comment

berhalak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

berhalak commented Aug 23, 2024

paulfitz commented Aug 23, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fflorent commented Sep 5, 2024

fflorent Aug 16, 2024 •

edited

Loading