#4953 Use test replaceable port #5887
Conversation
| @@ -1,6 +1,7 @@ | |||
| /* ©️ 2016 - present FlowCrypt a.s. Limitations apply. Contact [email protected] */ | |||
| import { readFileSync, writeFileSync } from 'fs'; | |||
| import { execSync as exec } from 'child_process'; | |||
| const MOCK_PORT = '[TEST_REPLACEABLE_MOCK_PORT]'; | |||
There was a problem hiding this comment.
We can't import the MOCK_PORT variable here as we do in other places (e.g., import { MOCK_PORT } from './const.js';) because this is a separate TypeScript project.
To address this, I inserted [TEST_REPLACEABLE_MOCK_PORT] and replaced it later in config-mock-build.sh.
However, I feel that the current implementation has made the code a bit more complex. Let me know if this aligns with your expectations or if you have any other suggestions.
There was a problem hiding this comment.
Yes, using separate MOCK_PORT variable adds some complexity, but it also removes a bit of confusion from generated original mock code where added :8001 should be replaced later again.
Your implementation looks good, let's continue with this approach.
Thanks for asking!
| @@ -80,7 +80,7 @@ export const getMockCustomerUrlFesEndpoints = (config: FesConfig | undefined): H | |||
| '/api/v1/message/FES-MOCK-EXTERNAL-ID/gateway': async ({ body }, req) => { | |||
| const port = parsePort(req); | |||
| if (parseAuthority(req) === standardFesUrl(port) && req.method === 'POST') { | |||
| // test: `compose - [email protected]:8001 - PWD encrypted message with FES web portal` | |||
| // test: `compose - [email protected]:{port} - PWD encrypted message with FES web portal` | |||
There was a problem hiding this comment.
let's just remove :{port} from tests names, so it'll be compose - [email protected] - PWD encrypted message with FES web portal, same for other tests
| @@ -23,7 +23,7 @@ | |||
| BROWSER_UNIT_TEST_NAME(`Sks lookup pubkey - trailing slash`); | |||
| (async () => { | |||
| const email = '[email protected]'; | |||
| const sks = new Sks('https://localhost:8001/'); | |||
| const sks = new Sks(`https://localhost:${MOCK_PORT}/`); | |||
There was a problem hiding this comment.
for unit tests we should leave fixed :8001, as different ports are used only in ui tests, so we can run multiple ui tests on different localhost ports
| @@ -1,6 +1,7 @@ | |||
| /* ©️ 2016 - present FlowCrypt a.s. Limitations apply. Contact [email protected] */ | |||
| import { readFileSync, writeFileSync } from 'fs'; | |||
| import { execSync as exec } from 'child_process'; | |||
| const MOCK_PORT = '[TEST_REPLACEABLE_MOCK_PORT]'; | |||
There was a problem hiding this comment.
Yes, using separate MOCK_PORT variable adds some complexity, but it also removes a bit of confusion from generated original mock code where added :8001 should be replaced later again.
Your implementation looks good, let's continue with this approach.
Thanks for asking!
|
Hmm.. for some reason, tests fail and some cases tests succeed. |
|
Hmm.. It now succeed everytime. |
|
Enterprise mock tests(15 tests) fail after 4 consequent succees checks. |
|
Digging more |
|
Seems like above one fixed issue. |
| const result = await asyncExec(`sh ./scripts/config-mock-build.sh ${buildDir} ${address.port}`); | ||
| const filepath = `${buildDir}/js/common/core/const.js`; | ||
|
|
||
| const fileContent = await readFile(filepath, { encoding: 'utf-8' }); | ||
| const updatedCode = fileContent.replace(/const (MOCK_PORT) = [^;]+;/g, `const $1 = '${address.port}';`); | ||
| await writeFile(filepath, updatedCode); |
There was a problem hiding this comment.
probably we can perform MOCK_PORT replacement in config-mock-build.sh script, like we do for TEST_REPLACEABLE_MOCK_PORT?
| @@ -159,7 +160,7 @@ export class Str { | |||
| if (email.includes(' ')) { | |||
| return false; | |||
| } | |||
| email = email.replace(/\:8001$/, ''); // for MOCK tests, todo: remove from production | |||
| email = email.replace(new RegExp(`:${MOCK_PORT}$`), ''); // for MOCK tests, todo: remove from production | |||
There was a problem hiding this comment.
is it still used or we can remove this replacement?
There was a problem hiding this comment.
I thought it is still used but let me check again
This PR used test replaceable port
close #4953 // if this PR closes an issue
Tests (delete all except exactly one):
To be filled by reviewers
I have reviewed that this PR... (tick whichever items you personally focused on during this review):