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

MYST-224 Refactor `identityHandler`, add tests #89

Merged
merged 6 commits into from Jan 10, 2018

Conversation

Projects
None yet
2 participants
@donce
Copy link
Contributor

commented Jan 9, 2018

I'm going to work around identityHandler for MYST-224, so it should have at least basic tests.

@@ -73,3 +75,15 @@ func NewCommandWith(
},
}
}

func SelectIdentity(identityHandler *identityHandler, keyOption string) (id identity.Identity, err error) {

This comment has been minimized.

Copy link
@Waldz

Waldz Jan 9, 2018

Member

Idea of handler was, to have thiner code in command.
This why, I moved it to identity_handler.go next to command.go.

This comment has been minimized.

Copy link
@donce

donce Jan 9, 2018

Author Contributor

well, SelectIdentity is independent function - maybe it's worth to move it to new file to make less code in this file?

This comment has been minimized.

Copy link
@Waldz

Waldz Jan 9, 2018

Member

Clear, decoupled from from handler struct.
This separate function could still be in node_handler.go

This comment has been minimized.

Copy link
@donce

donce Jan 10, 2018

Author Contributor

Moved it back to identity_handler.go as a function.

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

func Test_identityHandler_UseExisting(t *testing.T) {

This comment has been minimized.

Copy link
@Waldz

Waldz Jan 9, 2018

Member

+100


handler := NewNodeIdentityHandler(identityManager, client, cache)

id, err := handler.UseExisting("address")

This comment has been minimized.

Copy link
@Waldz

Waldz Jan 9, 2018

Member

But such value "address" is not mocked in identityManager. is not it?

This comment has been minimized.

Copy link
@donce

donce Jan 9, 2018

Author Contributor

This address is ignored in fake identity manager:

func (fakeIdm *idmFake) GetIdentity(_ string) (Identity, error) {
	return fakeIdm.FakeIdentity1, nil
}

This comment has been minimized.

Copy link
@Waldz

Waldz Jan 9, 2018

Member

But how can we use existing "address" if it dont exist actually. Could could double check, plz

This comment has been minimized.

Copy link
@donce

donce Jan 10, 2018

Author Contributor

Double checked - everything seems ok, not sure what's confusing you.

handler := NewNodeIdentityHandler(identityManager, client, cache)

id, err := handler.UseNew()
assert.Equal(t, identityManager.FakeIdentity2, id)

This comment has been minimized.

Copy link
@Waldz

Waldz Jan 9, 2018

Member

You should assert if it was registered via client

This comment has been minimized.

Copy link
@donce

donce Jan 9, 2018

Author Contributor

Done.

return identity, err
}
if !ic.cacheExists() {
err = errors.New("cache file does not exist")

This comment has been minimized.

Copy link
@Waldz

Waldz Jan 9, 2018

Member

It's not an error if file is missing. It's same as "identity non found in cache"

This comment has been minimized.

Copy link
@donce

donce Jan 9, 2018

Author Contributor
  1. This method is named GetIdentity and it seems a common sense to get an identity or an error.

  2. Looking in identity_handler.go:

	identity, err = ih.cache.GetIdentity()
	if err != nil || !ih.manager.HasIdentity(identity.Address) {
		return identity, errors.New("identity not found in cache")
	}

If cache file does not exist, this results in "identity not found in cache" error either way - with or without this change.
What this change is about is not only making method more predictable, but also avoiding calling
ih.manager.HasIdentity("") for empty identity, which is returned when cache does not exist. If we know that cache file does not exist, we should not check for an empty identity in identity manager.


handler := NewNodeIdentityHandler(identityManager, client, cache)

id, err := handler.UseExisting("address")

This comment has been minimized.

Copy link
@Waldz

Waldz Jan 9, 2018

Member

But how can we use existing "address" if it dont exist actually. Could could double check, plz

@@ -73,3 +75,15 @@ func NewCommandWith(
},
}
}

func SelectIdentity(identityHandler *identityHandler, keyOption string) (id identity.Identity, err error) {

This comment has been minimized.

Copy link
@Waldz

Waldz Jan 9, 2018

Member

Clear, decoupled from from handler struct.
This separate function could still be in node_handler.go

handler := NewNodeIdentityHandler(identityManager, client, cache)

id, err := handler.UseLast()
assert.Equal(t, fakeIdentity, id)

This comment has been minimized.

Copy link
@Waldz

Waldz Jan 9, 2018

Member

Could your assert that no identity was registered?

This comment has been minimized.

Copy link
@donce

donce Jan 10, 2018

Author Contributor

Done.

handler := NewNodeIdentityHandler(identityManager, client, cache)

id, err := handler.UseExisting("address")
assert.Equal(t, identityManager.FakeIdentity1, id)

This comment has been minimized.

Copy link
@Waldz

Waldz Jan 9, 2018

Member

Could your assert that no identity was registered?

This comment has been minimized.

Copy link
@donce

donce Jan 10, 2018

Author Contributor

Done.

@donce

This comment has been minimized.

Copy link
Contributor Author

commented Jan 10, 2018

All comments done.

@donce donce force-pushed the refactor/MYST-224-unlock-identity branch from 83a5b9a to 30d0397 Jan 10, 2018

@Waldz

Waldz approved these changes Jan 10, 2018

Copy link
Member

left a comment

LGTM

@Waldz Waldz merged commit 1f6e795 into master Jan 10, 2018

@Waldz Waldz deleted the refactor/MYST-224-unlock-identity branch Jan 10, 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.