Cleanup tests after app home internal url #982
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Opening this PR to know whether all the tests pass. |
fflorent
force-pushed
the
cleanup-tests-after-app-home-internal-url
branch
2 times, most recently
from
106b041 to
03a8c4d
Compare
fflorent
force-pushed
the
cleanup-tests-after-app-home-internal-url
branch
from
03a8c4d to
579ed39
Compare
fflorent
force-pushed
the
cleanup-tests-after-app-home-internal-url
branch
from
579ed39 to
354e177
Compare
fflorent
force-pushed
the
cleanup-tests-after-app-home-internal-url
branch
2 times, most recently
from
1be171e to
cb593da
Compare
fflorent
commented
Nov 5, 2024
| @@ -257,25 +249,16 @@ export class TestServerReverseProxy { | |||
| this._requireFromOutsideHeader = true; | |||
| } | |||
|
|
|||
| public async start(homeServer: TestServer, docServer: TestServer) { | |||
| this._app.all(['/dw/dw1', '/dw/dw1/*'], (oreq, ores) => this._getRequestHandlerFor(docServer)); | |||
There was a problem hiding this comment.
These endpoints seem to not have been called until I enabled the tests above. That's the reason why this error in the code had no impact until now (if I restore the old version I get tests that fail).
| await docs.start(home.serverUrl, { | ||
| ...additionalEnvConfiguration, | ||
| APP_DOC_URL: `${proxy.serverUrl}/dw/dw1`, | ||
| APP_DOC_INTERNAL_URL: docs.serverUrl, |
There was a problem hiding this comment.
I mentioned in the PR description that I had to rework the TestServer class. That's the reason why.
You can see that the serverUrl is now known before the server is started, which lets us provide the APP_DOC_INTERNAL_URL to the doc worker.
hexaltation
reviewed
Nov 6, 2024
fflorent
commented
Nov 20, 2024
| @@ -28,31 +27,36 @@ export class TestServer { | |||
| _homeUrl?: string, | |||
| options: {output?: Writable} = {}, // Pipe server output to the given stream | |||
| ): Promise<TestServer> { | |||
|
|
|||
| const server = new this(serverTypes, tempDirectory, suitename); | |||
| const port = await getAvailablePort(parseInt(process.env.GET_AVAILABLE_PORT_START || '8080', 10)); | |||
There was a problem hiding this comment.
Is GET_AVAILABLE_PORT_START used? getAvailablePort has a default value for its first argument (8000)
fflorent
force-pushed
the
cleanup-tests-after-app-home-internal-url
branch
from
7de1e98 to
d9598b7
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Context and proposed solution
This PR is a follow-up of PR #864, which introduced many tests to ensure that the DocApi can work behind a proxy, and especially ensures that the requests initiated internally do not pass through the proxy when
APP_HOME_INTERNAL_URLandAPP_DOC_INTERNAL_URLare set.We especially set
APP_DOC_INTERNAL_URLto the doc worker so until-now-skipped tests can be executed (namelydocument is protected during upload-and-import sequenceand/workspaces/{wid}/import handles empty filenames).TestServerandTestProxiedServerhave been reworked to achieve that, especially to separate the instanciation (during which a port is found for them) and their start.There are also other inconsistencies fixed, various cleanups, etc.
Tests?