-
Notifications
You must be signed in to change notification settings - Fork 314
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
Feature/implement e2e client connects to node test #204
Feature/implement e2e client connects to node test #204
Conversation
@@ -3,3 +3,4 @@ | |||
/vendor | |||
/.env | |||
testdataoutput | |||
e2e_tests.log |
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.
newline! :D
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.
Not again :) fixed.
… received through CMD
@@ -13,6 +13,9 @@ | |||
# copy update-resolv-conf to myst client current directory | |||
# build/client/ (by default) | |||
|
|||
#This check helps to avoid running this script in linux (win :)) based environments | |||
[[ $OSTYPE = "darwin"* ]] || exit 0 |
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 =
valid comparison operator?
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.
It's a special case when expanding os type and making string match (wildchar at the end) so yes this expression is valid.
@@ -5,10 +5,13 @@ OS_DIR_CONFIG="/etc/mysterium-client" | |||
OS_DIR_DATA="/var/lib/mysterium-client" | |||
OS_DIR_RUN="/var/run/mysterium-client" | |||
|
|||
mkdir -p $OS_DIR_RUN |
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.
Server entrypoint has
if [ ! -d "$OS_DIR_RUN" ]; then
mkdir -p $OS_DIR_RUN
fi
Would be good to unify both places
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.
Done
|
||
var identity client.IdentityDTO | ||
if len(identities) < 1 { | ||
identity, err = tequilApi.NewIdentity("") |
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.
It's 2nd green path. Should not we have 2 separate tests for that?
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.
It's not green path. It's ensurement that we have available identity to use for connection - test cannot proceed without that.
bin/run_e2e_tests
Outdated
cleanup | ||
exit 1 | ||
fi | ||
|
||
${dockerComposeCmd} build | ||
if [ ! $? -eq 0 ]; then | ||
print_error "Building docker images failed" | ||
exit 1 |
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.
Cleanup missing.
cleanup | ||
exit 1 | ||
fi | ||
|
||
${dockerComposeCmd} build |
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.
What if images are already built, won't this finish instantly while database is not set-up yet?
bin/run_e2e_tests
Outdated
if [ ! $? -eq 0 ]; then | ||
print_error "Db migration failed" | ||
cleanup | ||
exit 1 |
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.
These 3 lines repeats on each step - we can make a function fatal(error_message)
to DRY this.
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.
It's bash - next time
cmd/commands/cli/command_cli.go
Outdated
countryString = *country | ||
} else { | ||
countryString = "Unknown" | ||
if len(country) == 0 { |
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'm confusied - Isn't countryString
left empty if len(country)
!= 0?
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.
Fixed
@@ -25,14 +27,18 @@ type ProposalDTO struct { | |||
ServiceDefinition ServiceDefinitionDTO `json:"serviceDefinition"` | |||
} | |||
|
|||
func (p ProposalDTO) String() string { | |||
return fmt.Sprintf("Id: %d , Provider: %s, Country: %s", p.ID, p.ProviderID, p.ServiceDefinition.LocationOriginate.Country) |
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.
p.ServiceDefinition.LocationOriginate.Country
This is very long call chain and it seems to be repeated everywhere we need country. Perhaps service proposal or service definition could have country
shortcut method?
No description provided.