refactor: migrate tests to Vitest + TS #1202

jescalada · 2025-09-14T11:40:02Z

Fixes #1150, built on top of #1166.

Not all tests were perfectly refactored from Chai/Mocha -> Vitest. Some required partial or total rewriting to get them to play nice.

Some tests, specifically the proxy-related tests (proxy.test.ts, testProxy.test.ts and most importantly testProxyRoute.test.ts) have been very problematic for some reason - the tests in testPush.test.ts seem to fail in the CI when some of the proxy tests execute prior. They fail on the first execution locally, and then pass without issue (maybe a caching/cleanup problem).

Coverage has dropped somewhat to 80.5%, but we should see an increase after removing unused/untested code such as plugin stuff.

Note that the CI is failing due to the new Blue Oak license.

…e-ts-refactor-redone

Co-authored-by: j-k <dev@j-k.io> Signed-off-by: Juan Escalada <97265671+jescalada@users.noreply.github.com>

jescalada · 2025-11-03T16:33:30Z

@pGrovesy @grovesy This is the other big PR that could use a review! 🙏🏼

kriswest

A huge contribution (reading it took half a day!).

A few questions about type overrides as and whether some can be removed for type safety in the tests. The schema docs generator also needs to be run (type generator was run). There's also a few places you could use vitest's assert to simplify checks.

I spotted a number of things that aren't really within the scope of this PR and commented on them. If on reviewing them you agree, either you or I should create issues to resolve those.

.github/workflows/dependency-review.yml

config.schema.json

kriswest · 2025-11-10T15:51:52Z

src/config/index.ts

  const config = loadFullConfiguration();
+
+  if (!config.cookieSecret) {
+    throw new Error('cookieSecret is not set!');


We should possibly make cookieSecret required in the config schema. Currently none of the top-level properties are required... but then that lets you validate a file full of overrides (as long as the defaults cover everything that is actually required). However, its leading to our generated type for the config having these properties as optional.

This may be worth some further thought - we could have two top-level schemas (for the defaults and for override files) that reference the same defs for top-level properties I suppose. That would give us two different typescript types to use, as appropriate.

Setting these top-level properties as required ends up causing a bunch of ConfigLoader-related tests to fail because of the missing properties... I lean towards making them optional for flexibility since those test cases were likely based on actual use cases 🤔

Hence the comment about having two top-level schemas, one for the full config with all required elements required, and one for overrides... Makes maintenance more complex however. I wonder if we could achieve it in Typescript alone with either the Required<type> or Partial<type> utiulity types? I think wrapping Required around the current generated type would make all the top-level properties required and give us the type safety where we need it without extra schema?

WDYT?

kriswest · 2025-11-10T15:54:37Z

src/config/index.ts

+export const getSessionMaxAgeHours = (): number => {
  const config = loadFullConfiguration();
-  return config.sessionMaxAgeHours;
+  return config.sessionMaxAgeHours || 24;


As above - no default needed if it has to be in the defaults and we have a type expressing that.

I looked into setting default values for these in the schema config, but unfortunately the generated types don't enforce the defaults anywhere.

Maybe we could have a constants.ts file to centralize these - or perhaps a defaults.ts file to make sure these are consistent? Right now we have default values scattered between the proxy.config.json, config/index.ts and a few other spots as a fallback for the getSomeConfigValue() functions.

See previous comment about using Required. Types can't enforce the defaults, but they can enforce a value being set. If the value is required to be set (in a full configuration) then no default is needed. That could be a neat solution, i.e. functions like loadFullConfiguration returning Required<GitProxyConfig> - although they might need to confirm that all those values are set on load and throw an error if not (e.g. because someone messed with the default config).

src/proxy/processors/push-action/checkCommitMessages.ts

kriswest · 2025-11-11T13:05:02Z

test/testPush.test.ts

+            ],
+          },
+        });
+      expect(res.status).toBe(401);


This should probably be a 403 - not an issue for this PR

kriswest · 2025-11-11T13:05:31Z

test/testPush.test.ts

+            ],
+          },
+        });
+      expect(res.status).toBe(401);


This should probably be a 403

kriswest · 2025-11-11T13:05:51Z

test/testPush.test.ts

+            {
+              label: 'I am happy for this to be pushed to the upstream repository',
+              tooltip: {
+                text: 'Are you happy for this contribution to be pushed upstream?',
+                links: [],
+              },
+              checked: true,
+            },
+          ],
+        },
+      });
+    expect(res.status).toBe(401);


This should probably be a 403

kriswest · 2025-11-11T13:06:54Z

test/testPush.test.ts

+          ],
+        },
+      });
+    expect(res.status).toBe(401);


This should probably be a 403, etc. will stop marking them here

kriswest · 2025-11-11T13:15:48Z

vitest.config.ts

@@ -0,0 +1,35 @@
+import { defineConfig } from 'vitest/config';


Should there be a vitest config file for the cli tests as well?

Co-authored-by: Kris West <kristopher.west@natwest.com> Signed-off-by: Juan Escalada <97265671+jescalada@users.noreply.github.com>

…om/jescalada/git-proxy into 1150-vitest-migration-from-service

Co-authored-by: Kris West <kristopher.west@natwest.com> Signed-off-by: Juan Escalada <97265671+jescalada@users.noreply.github.com>

…om/jescalada/git-proxy into 1150-vitest-migration-from-service

Co-authored-by: Kris West <kristopher.west@natwest.com> Signed-off-by: Juan Escalada <97265671+jescalada@users.noreply.github.com>

…om/jescalada/git-proxy into 1150-vitest-migration-from-service

jescalada and others added 30 commits August 24, 2025 09:39

refactor(ts): passport/activeDirectory

d3d73d9

chore: add missing types

ba086f1

refactor(ts): JWT handler and utils

0c7d1fb

refactor(ts): passport/index

b419f4e

refactor(ts): passport/local

06a64ea

refactor(ts): passport/ldaphelper

4dfbc2d

refactor(ts): passport/oidc

09a1876

refactor(ts): auth routes

abc09bd

refactor(ts): config routes

03c4952

refactor(ts): misc routes and index

7ed9eb0

refactor(ts): push routes and update related types/db handlers

3d99de2

refactor(ts): repo routes

944e0b5

refactor(ts): user routes

6a7089f

refactor(ts): emailSender and missing implementation

6c9d3bf

refactor(ts): service/index and missing types

6899e4e

refactor(ts): urls

63c30a0

fix: failing tests due to incorrect imports

812a910

chore: update .eslintrc

9d5bdd8

chore: fix type checks

c951015

chore: fix CLI service imports

b046903

Merge branch 'main' of https://github.com/finos/git-proxy into servic…

97ad7c7

…e-ts-refactor-redone

chore: run npm format

9008ac5

test: add basic oidc tests and ignore openid-client type error on import

f36b3d1

test: increase testOidc and testPush coverage

51df315

test: improve push test coverage

f7ed291

test: add missing smtp tests

b2b1b14

Update .eslintrc.json

ae43800

Co-authored-by: j-k <dev@j-k.io> Signed-off-by: Juan Escalada <97265671+jescalada@users.noreply.github.com>

Update src/db/file/users.ts

17a8adf

Co-authored-by: j-k <dev@j-k.io> Signed-off-by: Juan Escalada <97265671+jescalada@users.noreply.github.com>

Update src/service/passport/jwtAuthHandler.ts

c7cf87e

Co-authored-by: j-k <dev@j-k.io> Signed-off-by: Juan Escalada <97265671+jescalada@users.noreply.github.com>

Update src/service/passport/index.ts

8aa1a97

Co-authored-by: j-k <dev@j-k.io> Signed-off-by: Juan Escalada <97265671+jescalada@users.noreply.github.com>

Merge branch 'main' into 1150-vitest-migration-from-service

2350b59

jescalada mentioned this pull request Oct 31, 2025

test: add ESM plugin tests #1265

Closed

kriswest mentioned this pull request Nov 3, 2025

3rd November 2025 - GitProxy Meeting Minutes #1259

Closed

20 tasks

Merge branch 'main' into 1150-vitest-migration-from-service

9951967

jescalada and others added 3 commits November 5, 2025 21:12

Merge branch 'main' into 1150-vitest-migration-from-service

8e38584

Merge branch 'main' into 1150-vitest-migration-from-service

ea1f875

Merge branch 'main' into 1150-vitest-migration-from-service

1a42226

kriswest requested changes Nov 11, 2025

View reviewed changes

jescalada added 2 commits November 18, 2025 13:45

Merge branch 'main' into 1150-vitest-migration-from-service

d11c657

chore: fix dep issues

e845f1a

jescalada mentioned this pull request Nov 18, 2025

Improve logging in processors/action chain #1281

Open

jescalada and others added 18 commits November 18, 2025 09:05

Update test/1.test.ts

b741bfb

Co-authored-by: Kris West <kristopher.west@natwest.com> Signed-off-by: Juan Escalada <97265671+jescalada@users.noreply.github.com>

chore: remove unnecessary logging in push actions and routes

a53eeef

Merge branch '1150-vitest-migration-from-service' of https://github.c…

d572df1

…om/jescalada/git-proxy into 1150-vitest-migration-from-service

chore: remove/adjust tests based on console logs

890d583

chore: remove casting in ConfigLoader tests

5ae5c50

Update test/generated-config.test.ts

37c922a

Co-authored-by: Kris West <kristopher.west@natwest.com> Signed-off-by: Juan Escalada <97265671+jescalada@users.noreply.github.com>

Update test/generated-config.test.ts

a8aa410

Co-authored-by: Kris West <kristopher.west@natwest.com> Signed-off-by: Juan Escalada <97265671+jescalada@users.noreply.github.com>

Update test/generated-config.test.ts

5327e28

Co-authored-by: Kris West <kristopher.west@natwest.com> Signed-off-by: Juan Escalada <97265671+jescalada@users.noreply.github.com>

Merge branch '1150-vitest-migration-from-service' of https://github.c…

9723c02

…om/jescalada/git-proxy into 1150-vitest-migration-from-service

test: improve assertions in generated-config.test.ts

973fbaf

fix: set isEmailAllowed regex to case insensitive

c642e4d

chore: improve test assertions and cleanup

e7ffacb

test: simplify scanDiff tests with generateDiffStep helper

b06d61e

Update test/testParseAction.test.ts

119bd8b

Co-authored-by: Kris West <kristopher.west@natwest.com> Signed-off-by: Juan Escalada <97265671+jescalada@users.noreply.github.com>

Update test/testDb.test.ts

a4809b4

Co-authored-by: Kris West <kristopher.west@natwest.com> Signed-off-by: Juan Escalada <97265671+jescalada@users.noreply.github.com>

Update test/testCheckUserPushPermission.test.ts

e69c4d6

Co-authored-by: Kris West <kristopher.west@natwest.com> Signed-off-by: Juan Escalada <97265671+jescalada@users.noreply.github.com>

test: rename auth routes tests and add new cases for status codes

594b8aa

Merge branch '1150-vitest-migration-from-service' of https://github.c…

5ada78a

…om/jescalada/git-proxy into 1150-vitest-migration-from-service

		@@ -0,0 +1,35 @@
		import { defineConfig } from 'vitest/config';

refactor: migrate tests to Vitest + TS #1202

Are you sure you want to change the base?

refactor: migrate tests to Vitest + TS #1202

Uh oh!

Conversation

jescalada commented Sep 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jescalada commented Nov 3, 2025

Uh oh!

kriswest left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jescalada commented Sep 14, 2025 •

edited

Loading