Skip to content
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/myst 684 e2e tests registers identity #323

Merged
merged 35 commits into from Sep 11, 2018

Conversation

Projects
None yet
4 participants
@zolia
Copy link
Member

commented Aug 28, 2018

No description provided.

@zolia zolia requested review from tadovas, Waldz and soffokl Aug 28, 2018

Gopkg.lock Outdated
]
pruneopts = "UT"

This comment has been minimized.

Copy link
@soffokl

soffokl Aug 28, 2018

Member

@tadovas figured out that dep version 0.5.0 or higher required to avoid all these unnecessary changed. I'm on the way to add it to some description.

@tadovas
Copy link
Member

left a comment

Main considerations:

  1. Too fat registration data endpoint
  2. Complicated registration event wait
Gopkg.lock Outdated
]
pruneopts = "UT"

This comment has been minimized.

Copy link
@tadovas

tadovas Aug 28, 2018

Member

Please update your dep to at least 0.5.0

go cmd.discoveryAnnouncementLoop(proposal, cmd.mysteriumClient, signer, stopDiscoveryAnnouncement)

// if not registered - wait indefinitely for identity registration event
if !registered {

This comment has been minimized.

Copy link
@tadovas

tadovas Aug 28, 2018

Member

This looks very complicated and could possibly refactored to separate function loop as:

  1. check registration
  2. wait if needed
  3. register proposal
  4. ping proposal
  5. unregister proposal
    It could be discovery component of its own

This comment has been minimized.

Copy link
@zolia

zolia Aug 30, 2018

Author Member

true, reworked into event loop. IMHO looks better


return nil
}

func (cmd *Command) registerTequilAPI(statusProvider registry.IdentityRegistry, providerID identity.Identity, registrationDataProvider registry.RegistrationDataProvider) error {

This comment has been minimized.

Copy link
@tadovas

tadovas Aug 28, 2018

Member

startServingTequilAPI ?

This comment has been minimized.

Copy link
@tadovas

tadovas Aug 28, 2018

Member

Also this function does a bit too much - registers endpoints on routers and starts http server. It could be split into two methods - one for endpoints registering, another for starting http

This comment has been minimized.

Copy link
@Waldz

Waldz Aug 29, 2018

Member

Dont need this function at all #324 implements Tequilapi on VPN server

This comment has been minimized.

Copy link
@zolia

zolia Aug 31, 2018

Author Member

removed, now node contains only former mysterium_client tequilapi.
Removed /identity/current endpoint

return nil
}

func (cmd *Command) identityRegistrationWaitLoop(providerID identity.Identity, identityRegistry registry.IdentityRegistry, stopLoop chan int) bool {

This comment has been minimized.

Copy link
@tadovas

tadovas Aug 28, 2018

Member

As I mentioned earlier - it could be separate discovery component which takes care of proposal registration, checking own identity registration is part of that.

}
log.Info("registration wait loop stopped")
return true
case <-time.After(60 * time.Minute):

This comment has been minimized.

Copy link
@tadovas

tadovas Aug 28, 2018

Member

As I understand wait for registration should wait indefinetely - it's no point to fail after 1h

)

router.GET("/identities/:id/registration", registrationEndpoint.RegistrationData)
router.GET("/identity/registration", registrationEndpoint.OwnRegistrationData)

This comment has been minimized.

Copy link
@tadovas

tadovas Aug 28, 2018

Member

Please consider reworking registration data as suggested above

// IdentityRegistry checks whenever given identity is registered
var ErrNoIdentityRegisteredTimeout = errors.New("no identity registration, waiting for registration")

type Register struct {

This comment has been minimized.

Copy link
@tadovas

tadovas Aug 28, 2018

Member

BlockchainIdentityRegistry ?

}

// WaitForRegistrationEvent returns registeredEvent if given providerAddress was registered within payments contract
func (register *Register) WaitForRegistrationEvent(providerAddress common.Address, registeredEvent chan int, stopLoop chan int) {

This comment has been minimized.

Copy link
@tadovas

tadovas Aug 28, 2018

Member

Please redesign function as mentioned above (comment about function signature)

}

// PrintRegistrationData prints identity registration data needed to register identity with payments contract
func PrintRegistrationData(data *registry.RegistrationData) {

This comment has been minimized.

Copy link
@tadovas

tadovas Aug 28, 2018

Member

This presentation style function has nothing to do with registry package

Show resolved Hide resolved tequilapi/http_api_server.go Outdated

@zolia zolia force-pushed the feature/MYST-684-e2e-tests-registers-identity branch from 564c8ac to 8028cd6 Aug 28, 2018

zolia added some commits Aug 21, 2018

@@ -357,7 +357,7 @@ func (c *Command) identities(argsString string) {
}

func (c *Command) registration(argsString string) {
status, err := c.tequilapi.RegistrationStatus(argsString)
status, err := c.tequilapi.IdentityRegistrationStatus(argsString)

This comment has been minimized.

Copy link
@Waldz

Waldz Aug 29, 2018

Member

Suggest to specify what variable is argsString


return nil
}

func (cmd *Command) registerTequilAPI(statusProvider registry.IdentityRegistry, providerID identity.Identity, registrationDataProvider registry.RegistrationDataProvider) error {

This comment has been minimized.

Copy link
@Waldz

Waldz Aug 29, 2018

Member

Dont need this function at all #324 implements Tequilapi on VPN server

@@ -75,6 +78,19 @@ func ParseArguments(args []string) (options CommandOptions, err error) {
"Openvpn port to use. Default 1194",
)

flags.StringVar(

This comment has been minimized.

Copy link
@Waldz

Waldz Aug 29, 2018

Member

Stale


err := waiter.myAddress.Connect()
if err != nil {
return dto_discovery.Contact{}, fmt.Errorf("failed to start my connection. %s", waiter.myAddress)
return dto_discovery.Contact{}, log.Error(waiterLogPrefix, "failed to start my connection with: ", waiter.myAddress.GetContact())

This comment has been minimized.

Copy link
@Waldz

Waldz Aug 29, 2018

Member

But we dont need to log here.
Just format and return error

@@ -19,8 +19,9 @@ $dockerComposeCmd logs -f > e2e_tests.log &

$dockerComposeCmd run go-runner \
go test -v ./e2e/... -args \
--tequila.host=client \
--tequila.client-host=client \
--tequila.port=4050 \

This comment has been minimized.

Copy link
@Waldz

Waldz Aug 29, 2018

Member

Port should also be separate

@@ -39,14 +40,14 @@ func TestClientConnectsToNode(t *testing.T) {
err = tequilApi.Unlock(identity.Address, "")
assert.NoError(t, err)

registrationData, err := tequilApi.RegistrationStatus(identity.Address)
registrationData, err := tequilApi.IdentityRegistrationStatus(identity.Address)

This comment has been minimized.

Copy link
@Waldz

Waldz Aug 29, 2018

Member

Asserting just fatal errors, I would assert structure of registrationData here

This comment has been minimized.

Copy link
@zolia

zolia Aug 30, 2018

Author Member

I can assert only registration fact from this structure as its always new identity being generated.

@@ -59,6 +60,8 @@ type RegistrationDataDTO struct {
// Returns true if identity is registered in payments smart contract
Registered bool `json:"registered"`

Address string `json:"address"`

This comment has been minimized.

Copy link
@Waldz

Waldz Aug 29, 2018

Member

Caller already knows address, why to repeat it?

This comment has been minimized.

Copy link
@Waldz

Waldz Sep 10, 2018

Member

Fixed

}

func newRegistrationEndpoint(dataProvider RegistrationDataProvider, statusProvider IdentityRegistry) *registrationEndpoint {
func newRegistrationEndpoint(dataProvider RegistrationDataProvider, statusProvider IdentityRegistry, identity *identity.Identity) *registrationEndpoint {

This comment has been minimized.

Copy link
@Waldz

Waldz Aug 29, 2018

Member

I agree.
And soon we will have endpoint PUT /service { providerId: "0x1" } and caller will know his current identity

@@ -81,6 +81,7 @@ func TestRegistrationEndpointReturnsRegistrationData(t *testing.T) {
t,
`{
"registered" : false,
"address":"0x1231323131",

This comment has been minimized.

Copy link
@Waldz

Waldz Aug 29, 2018

Member

excess data

This comment has been minimized.

Copy link
@Waldz

Waldz Sep 10, 2018

Member

Fixed

Show resolved Hide resolved service_discovery/dto/location_test.go

@zolia zolia force-pushed the feature/MYST-684-e2e-tests-registers-identity branch from 8028cd6 to 22a606c Aug 30, 2018

@zolia zolia force-pushed the feature/MYST-684-e2e-tests-registers-identity branch from 423bf4c to 73fff6a Aug 30, 2018

@zolia zolia force-pushed the feature/MYST-684-e2e-tests-registers-identity branch from e7e6d3d to d421721 Aug 31, 2018

Merge branch 'master' of github.com:MysteriumNetwork/node into featur…
…e/MYST-684-e2e-tests-registers-identity

@zolia zolia force-pushed the feature/MYST-684-e2e-tests-registers-identity branch from 5d7fb27 to f72d9e9 Sep 4, 2018

zolia added some commits Sep 3, 2018

@zolia zolia force-pushed the feature/MYST-684-e2e-tests-registers-identity branch from f72d9e9 to b9a7a58 Sep 4, 2018


// Register type combines different IdentityRegistry interfaces under single type
type Register struct {
callerSession *generated.IdentityRegistryCallerSession

This comment has been minimized.

Copy link
@Waldz

Waldz Sep 5, 2018

Member

Composition of interfaces, should not You extend both?

@zolia zolia force-pushed the feature/MYST-684-e2e-tests-registers-identity branch from c628615 to ebf9ddd Sep 5, 2018

@zolia zolia force-pushed the feature/MYST-684-e2e-tests-registers-identity branch from ebf9ddd to 9dafd04 Sep 5, 2018

zolia and others added some commits Sep 5, 2018

Merge branch 'master' of github.com:MysteriumNetwork/node into featur…
…e/MYST-684-e2e-tests-registers-identity

* 'master' of github.com:MysteriumNetwork/node:
  Fixed of building docker images for Ubuntu
  Clean up for renaming
  build-artifacts repo rename for Travis CI
  Renamed payments repo and vendored new version
  Reverted import of payments repo
  MYST-178. Fixed import path in packages
  Fixed some misspells. #308

Waldz added some commits Sep 9, 2018

@Waldz Waldz force-pushed the feature/MYST-684-e2e-tests-registers-identity branch from 0441dcf to dc1f2cd Sep 10, 2018

--tequila.port=4050 \
--geth.url=http://local-node:8545
--consumer.tequilapi-host=myst-consumer \
--consumer.tequilapi-port=4050 \

This comment has been minimized.

Copy link
@tadovas

tadovas Sep 11, 2018

Member

Interesting formatting. Github display issue?

This comment has been minimized.

Copy link
@Waldz

Waldz Sep 11, 2018

Member

Fixed

@soffokl soffokl force-pushed the feature/MYST-684-e2e-tests-registers-identity branch from b344b5c to f3f402f Sep 11, 2018

@@ -0,0 +1,197 @@
/*

This comment has been minimized.

Copy link
@tadovas

tadovas Sep 11, 2018

Member

Interesting file. Merge/rebase leftover?

@@ -0,0 +1,196 @@
/*

This comment has been minimized.

Copy link
@tadovas

tadovas Sep 11, 2018

Member

Still looks a bit complicated, but hey - finally discovery is isolated to separate package 👍


func (d *Discovery) stopLoop() {
log.Info(logPrefix, "stopping discovery loop..")
d.RLock()

This comment has been minimized.

Copy link
@tadovas

tadovas Sep 11, 2018

Member

Why a simple defer d.RUnlock() doesn't help here?

This comment has been minimized.

Copy link
@Waldz

Waldz Sep 11, 2018

Member

Will not work like that. And there is test to proove that

"github.com/stretchr/testify/assert"
)

func TestClientConnectsToNode(t *testing.T) {
var (

This comment has been minimized.

Copy link
@tadovas

tadovas Sep 11, 2018

Member

Isn't these values passed as e2e test args?

This comment has been minimized.

Copy link
@Waldz

Waldz Sep 11, 2018

Member

Cant reuse at the moment

service
--openvpn.port=3000

This comment has been minimized.

Copy link
@tadovas

tadovas Sep 11, 2018

Member

Custom openvpn port was a nice test, to make sure that port is nowhere hardcoded in our consumer/provider code. Why to remove it?

This comment has been minimized.

Copy link
@Waldz

Waldz Sep 11, 2018

Member

Reverted


func newTequilapiClient(domain Domain) *tequilapi_client.Client {
switch domain {
case Consumer:

This comment has been minimized.

Copy link
@tadovas

tadovas Sep 11, 2018

Member

Why we need those two constants? As I mentioned earlier, two separate functions with clear names maybe would be more readable, i.e. NewConsumerTequilApi, NewProviderTequilApi

This comment has been minimized.

Copy link
@Waldz

Waldz Sep 11, 2018

Member

Made 2 different factories

@@ -0,0 +1,11 @@
version: '3'

This comment has been minimized.

Copy link
@tadovas

tadovas Sep 11, 2018

Member

Nice. Suggestion: it would be awesome add e2e_test --debug flag, which will include this yml automatically, and also would wait for user input before tearing everything down.

This comment has been minimized.

Copy link
@Waldz

Waldz Sep 11, 2018

Member

Implemented

Waldz and others added some commits Sep 9, 2018

@Waldz Waldz force-pushed the feature/MYST-684-e2e-tests-registers-identity branch from f3f402f to 6dec1c5 Sep 11, 2018

@Waldz Waldz dismissed their stale review via 576cc47 Sep 11, 2018

@Waldz

Waldz approved these changes Sep 11, 2018

@Waldz Waldz merged commit 992c677 into master Sep 11, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@Waldz Waldz deleted the feature/MYST-684-e2e-tests-registers-identity branch Sep 11, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.