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.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
#4953 Use test replaceable port #5887
base: master
Are you sure you want to change the base?
#4953 Use test replaceable port #5887
Changes from 1 commit
c54528b
9131f4a
94d170d
34dcaf5
9fc50ad
b8e66df
d7a2022
915610f
017ac27
807d726
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is it still used or we can remove this replacement?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought it is still used but let me check again
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Checked and this logic is still used
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's just remove
:{port}
from tests names, so it'll becompose - [email protected] - PWD encrypted message with FES web portal
, same for other testsThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 portsThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 inconfig-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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sosnovsky
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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!