Skip to content

Refactor connection setup in vitest and fix connection cache - #3458

Open
SanjulaGanepola wants to merge 5 commits into
masterfrom
refactor/test-framework
Open

SanjulaGanepola wants to merge 5 commits into
masterfrom
refactor/test-framework

Conversation

@SanjulaGanepola

Copy link
Copy Markdown
Member

Changes

This PR refactors the connection setup code used in vitest so it is more structured and includes more setup logs. It also addresses an issue where the cache in .storage.json and .config.json was being re-used even though the connection in the .env file was different than what was cached.

How to test this PR

  1. Run npm run test

Checklist

  • have tested my change

Signed-off-by: Sanjula Ganepola <Sanjula.Ganepola@ibm.com>
Signed-off-by: Sanjula Ganepola <Sanjula.Ganepola@ibm.com>
@SanjulaGanepola

Copy link
Copy Markdown
Member Author

I am still in the process of finding a machine we can use for our CI. The machine I had switched to originally is too slow.

@github-actions

github-actions Bot commented Sep 21, 2026 •

Copy link
Copy Markdown
Contributor

👋 A new build is available for this PR based on be12e17.

@buzzia2001

buzzia2001 commented Oct 4, 2026 •

Copy link
Copy Markdown
Member

@SanjulaGanepola thanks for the refactor. I found two things that look blocking, plus a few smaller ones. We have also a conflict to be fixed...

  1. SSH agent auth is dropped (src/api/tests/setup/connection.ts, credentials). The old ENV_CREDS set useSshAgent: process.env.VITE_USE_SSH_AGENT === 'true'; the new credentials object no longer passes it. With only VITE_USE_SSH_AGENT=true set, env validation passes but IBMi.connect never takes the agent branch, so global setup and every suite fail with "Failed to connect to IBM i", including the SSH agent test itself.

  2. tools/sqlSignature.ts still imports the old path (line 84). It does await import('../src/api/tests/connection'), which this PR moves to src/api/tests/setup/connection. npm run sqlsignature will fail with "Cannot find module", and because the import is dynamic, type-checking doesn't catch it.

  3. Unguarded readdirSync(mapepireDistDir) in newConnection(). If dist/ doesn't exist (fresh clone, CI before build), this throws a raw ENOENT instead of the intended "Failed to locate Mapepire Server JAR" error. It also makes the build a hard precondition for connecting, which it wasn't before.

  4. Skip-setup check is too weak (setup.ts). It only checks that an entry with the connection name exists. A stale cache (different port, upgraded system) still matches, so setup is skipped and the suites run on outdated server settings.

  5. Connection name changed with no cleanup. It goes from ${host}_${user}_test to ${user}@${host}, so old entries stay orphaned in .config.json / .storage.json and get re-saved on every disposeConnection.

  6. parseInt results aren't validated (env.ts). VITE_CONNECTION_TIMEOUT=25s or a non-numeric VITE_DB_PORT becomes NaN and is passed on as the hook timeout or the ssh2 port, instead of producing a validation error like the other variables.

Nice to have...

  1. The ${VITE_DB_USER}@${VITE_SERVER} name template is duplicated in setup.ts and connection.ts. If the two drift, the match check is always false and a full setup runs every time without any error. Exporting it once would avoid that.

  2. setup() creates a second JsonStorage/JsonConfig pair on top of the module-level ones in connection.ts, and logEnvironmentVariables() re-parses the env although envVars is already imported.

  3. mapepireJarFileName is only used as an existence check, and the regex accepts a JAR of a different version than the one the Mapepire component expects. It also re-scans dist on every newConnection(); a single check in global setup would be enough.

  4. The missing-credentials message lists VITE_DB_PORT as required, but it's optional (defaults to 22). The empty exported teardown() in setup.ts looks like dead code.

@buzzia2001 buzzia2001 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rc

Signed-off-by: Sanjula Ganepola <Sanjula.Ganepola@ibm.com>
Signed-off-by: Sanjula Ganepola <Sanjula.Ganepola@ibm.com>
@SanjulaGanepola

Copy link
Copy Markdown
Member Author

All points have been addressed. Some comments:

  1. Skip-setup check is too weak (setup.ts). It only checks that an entry with the connection name exists. A stale cache (different port, upgraded system) still matches, so setup is skipped and the suites run on outdated server settings.

This is the same behavior as before. The tests can't know when something has been upgraded on the system. So it would be up to the person running the tests to clear the cache before running the tests again.

  1. Connection name changed with no cleanup. It goes from ${host}_${user}_test to ${user}@${host}, so old entries stay orphaned in .config.json / .storage.json and get re-saved on every disposeConnection.

This only impacts any previous tests runs locally before this PR. This can be avoided by just deleting the file. Do you prefer reverting back to _test? I just dropped it for consistency with what we do in production (not that it really matters).

setup() creates a second JsonStorage/JsonConfig pair on top of the module-level ones in connection.ts, and logEnvironmentVariables() re-parses the env although envVars is already imported.

I don't quite follow. The setup and connection cannot re-use the same. Setup is only a setup script run in vitest.config.ts. I also don't quite understand the re-parsing issue.

@SanjulaGanepola
SanjulaGanepola deployed to testing_environment October 5, 2026 15:15 — with GitHub Actions Active

@buzzia2001 buzzia2001 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the quick turnaround. Agreed on 4 and 5: the new check is already stricter than the old file-existence one, and the rename only affects local caches, so no need to go back to _test.
One thing is still open: tools/sqlSignature.ts line 84 still imports ../src/api/tests/connection, which no longer exists after the move to setup/connection, so npm run sqlsignature breaks.

node:internal/modules/esm/resolve:274
throw new ERR_MODULE_NOT_FOUND(
^

Error [ERR_MODULE_NOT_FOUND]: Cannot find module 'C:\Users\andrea.buzzi\Git\Code4i\Core\src\api\tests\connection' imported from C:\Users\andrea.buzzi\Git\Code4i\Core\tools\sqlSignature.ts
at finalizeResolution (node:internal/modules/esm/resolve:274:11)
at moduleResolve (node:internal/modules/esm/resolve:864:10)
at defaultResolve (node:internal/modules/esm/resolve:990:11)
at #cachedDefaultResolve (node:internal/modules/esm/loader:737:20)
at #resolveAndMaybeBlockOnLoaderThread (node:internal/modules/esm/loader:773:38)
at nextStep (node:internal/modules/customization_hooks:189:26)
at resolveBaseSync (file:///C:/Users/andrea.buzzi/AppData/Local/npm-cache/_npx/fd45a72a545557e9/node_modules/tsx/dist/register-nyXW-TH3.mjs:2:11092)
at resolveDirectorySync (file:///C:/Users/andrea.buzzi/AppData/Local/npm-cache/_npx/fd45a72a545557e9/node_modules/tsx/dist/register-nyXW-TH3.mjs:2:12398)
at resolveTsPathsSync (file:///C:/Users/andrea.buzzi/AppData/Local/npm-cache/_npx/fd45a72a545557e9/node_modules/tsx/dist/register-nyXW-TH3.mjs:2:13605)
at resolve (file:///C:/Users/andrea.buzzi/AppData/Local/npm-cache/_npx/fd45a72a545557e9/node_modules/tsx/dist/register-nyXW-TH3.mjs:2:16832) {
code: 'ERR_MODULE_NOT_FOUND',
url: 'file:///C:/Users/andrea.buzzi/Git/Code4i/Core/src/api/tests/connection'
}

This branch was successfully deployed

1 active deployment
testing_environment — be12e17d Deployed Oct 5, 2026 by SanjulaGanepola via Test runner #704
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants