Jail.RemoveCells before logout and switching account #382

oleg-raev · 2017-10-05T15:48:10Z

Added methods:

Jail.RemoveCells
JailCell.Stop
Loop.Stop

Added call of method RemoveCells at begin of Logout action and begin of SelectAccount action of geth API

Added tests:

(api_test.go) TestJailRemoveCells
(api_test.go) TestLogoutRemovesCells
(jail_test.go) TestJailRemoveCells
(jail_cell_test.go) TestJailCellStopSetIntervalBeforeStop
(jail_cell_test.go) TestJailCellStopSetIntervalAfterStop
(jail_cell_test.go) TestJailCellStopTwice

Issue #260 - setInterval keeps on posting updates after the account has been switched

… call of method RemoveCells at begin of Logout action and begin of SelectAccount action of geth API

divan · 2017-10-05T22:24:32Z

@oleg-raev thanks for terrific work. This code is not easy to grasp quickly.
However, this functionality already has been implemented in https://github.com/status-im/status-go/pull/376/commits, and we're going to merge it today. The approach there is similar to yours, but uses context package for cancelling cells. Also that PR doesn't touch API part for now. What I really like is a test/code ratio, and I'd definitely revisit it after merging (we going to merge a bunch of pending refactoring branches).

For now, marking this as blocked by #376

oleg-raev · 2017-10-07T12:23:10Z

@divan Thank you for your update and quick merging your PR.

The approach there is similar to yours, but uses context package for cancelling cells

Yes, it looks better and easier.

I should resolve conflicts and update the pull request, right?
What about failing check? Is it a big problem?

divan · 2017-10-07T15:09:37Z

I should resolve conflicts and update the pull request, right?

Umm, not yet, the changes were huge (including file renames). We're about to merge another PR, related to tests. Once it's done, we'll revisit this PR and probably will manually cherry pick some changes (like logout API, and especially tests).

tiabc · 2017-10-09T17:07:14Z

Thank you very much for the contribution, @oleg-raev!

We may either cherry-pick needed commits from your branch manually, or you can groom this branch yourself after #375 is merged if you have time. Which one is preferred for you?

Added a zenhub dependency on #371 as per @divan's comment.
Also removing the blocked label as blocking state is seen from zenhub.

oleg-raev · 2017-10-10T02:39:22Z

Your welcome, @tiabc

We may either cherry-pick needed commits from your branch manually

There is only one commit.

you can groom this branch yourself after #375 is merged if you have time

Yes. Let's go this way.

tiabc · 2017-10-11T14:24:01Z

@oleg-raev please, proceed.

oleg-raev · 2017-10-15T15:35:40Z

@tiabc conflicts resolved

tiabc

Everything's great, just a number of minor fixes or questions. Can you fix or answer them, please?

tiabc · 2017-10-16T13:57:24Z

e2e/api/api_test.go

+)
+
+var (
+	TestConfig *common.TestConfig


It's already here:

status-go/testing/testing.go

Line 16 in 281b304

TestConfig *common.TestConfig

We use it like this:

status-go/e2e/accounts/accounts_test.go

Line 12 in 281b304

. "github.com/status-im/status-go/testing"

tiabc · 2017-10-16T13:58:29Z

e2e/api/api_test.go

@@ -25,8 +35,15 @@ type APITestSuite struct {
 }

 func (s *APITestSuite) SetupTest() {
+	var err error


Let's more it closer to where it's actually used. To line 43.
(if it won't be removed due to the first comment, of course)

tiabc · 2017-10-16T14:10:48Z

e2e/api/api_test.go

@@ -118,3 +135,85 @@ func (s *APITestSuite) TestRaceConditions() {
 	time.Sleep(2 * time.Second) // so that we see some logs
 	s.api.StopNode()            // just in case we have a node running
 }
+
+func (s *APITestSuite) TestJailStop() {


TestCellsRemovedAfterSwitchAccount

tiabc · 2017-10-16T14:11:09Z

e2e/api/api_test.go

+
+	for i := 0; i < itersCount; i++ {
+		wg.Add(1)
+		go func(id int) {


Why doing it in a goroutine?

tiabc · 2017-10-16T14:12:17Z

e2e/api/api_test.go

+	s.api.JailManager().Parse(testChatID, ``)
+
+	_, err = s.api.JailManager().NewCell(testChatID)
+	require.NoError(err)


Sorry, what's the meaning of this assertion?

Exactly, you are right. Cell created by previous call s.api.JailManager().Parse(...

tiabc · 2017-10-16T14:23:27Z

e2e/jail/jail_test.go

+
+func (s *JailTestSuite) TestJailStop() {
+	const loopLen = 5
+	var wg sync.WaitGroup


Again: why goroutines?

tiabc · 2017-10-16T14:24:49Z

e2e/jail/jail_test.go

+
+	c := make(chan struct{})
+	go func() {
+		defer close(c)


Won't it be easier to read without defer?

tiabc · 2017-10-16T14:25:09Z

e2e/jail/jail_test.go

@@ -157,3 +159,49 @@ func (s *JailTestSuite) TestEventSignal() {
 	expectedResponse := `{"jsonrpc":"2.0","result":true}`
 	s.Equal(expectedResponse, response)
 }
+
+func (s *JailTestSuite) TestJailStop() {


TestJailCellsRemovedAfterStop

tiabc · 2017-10-16T14:38:16Z

geth/api/api.go

@@ -146,11 +146,13 @@ func (api *StatusAPI) VerifyAccountPassword(keyStoreDir, address, password strin
 // using provided password. Once verification is done, decrypted key is injected into Whisper (as a single identity,
 // all previous identities are removed).
 func (api *StatusAPI) SelectAccount(address, password string) error {
+	api.b.jailManager.Stop()


Hm. Currently JailManager.Stop doesn't stop Jail, it just resets it to an initial state. Can you make a comment above the original method like // FIXME(oleg-raev): This method doesn't make stop, it rather resets its cells to an initial state and should be properly renamed, for example: ResetCells?

tiabc · 2017-10-16T14:38:52Z

geth/jail/cell_test.go

@@ -19,6 +20,10 @@ var (
 	baseStatusJSCode = string(static.MustAsset("testdata/jail/status.js"))
 )

+func TestCellTestSuite(t *testing.T) {
+	suite.Run(t, new(CellTestSuite))
+}


oleg-raev · 2017-10-16T15:26:09Z

@tiabc fixed

tiabc · 2017-10-16T15:33:22Z

geth/api/api.go

@@ -146,11 +146,15 @@ func (api *StatusAPI) VerifyAccountPassword(keyStoreDir, address, password strin
 // using provided password. Once verification is done, decrypted key is injected into Whisper (as a single identity,
 // all previous identities are removed).
 func (api *StatusAPI) SelectAccount(address, password string) error {
+	// FIXME(oleg-raev): This method doesn't make stop, it rather resets its cells to an initial state
+	// and should be properly renamed, for example: ResetCells


I meant to the method declaration. But never mind, it's fine.

influx6

LGTM!

themue

LGTM

tiabc · 2017-10-16T21:05:42Z

Merging without another review as this PR has been hanging too long.
There's a failure in CI caused by a timeout: https://travis-ci.org/status-im/status-go/builds/288617791#L527
These tests pass locally and I'll file a PR for this particular timeout soon.

tiabc · 2017-10-16T21:08:36Z

@oleg-raev thanks one more time for the successful contribution! We'll have a conversation about you joining core contributors and I'll get back to you with the result.

Added method Jail.RemoveCells, method JailCell.Stop, Loop.Stop. Added…

39a87e5

… call of method RemoveCells at begin of Logout action and begin of SelectAccount action of geth API

oleg-raev changed the title ~~Added method Jail.RemoveCells, method JailCell.Stop, Loop.Stop. Added…~~ Jail.RemoveCells before logout and switching account Oct 5, 2017

divan added the blocked label Oct 5, 2017

tiabc removed the blocked label Oct 9, 2017

tiabc mentioned this pull request Oct 9, 2017

Add Start and Stop Methods to Jail #379

Closed

Merge remote-tracking branch 'upstream/develop' into issues/260

2667815

oleg-raev mentioned this pull request Oct 14, 2017

Extract e2e tests to a separate package #375

Merged

Oleg Raev added 4 commits October 14, 2017 20:28

removed duplicated in fact test

7ec6b7f

added suite init

0bb7df2

added test for Jail Stop action

c65464f

fixed api_test

5282ec2

oleg-raev mentioned this pull request Oct 15, 2017

Add initial support for cancellation context in Jail #376

Merged

Oleg Raev added 3 commits October 15, 2017 22:17

fixed jail.cells reset

005483c

removed superfluous conditions for jail stopping

f495d4d

Merge remote-tracking branch 'upstream/develop' into issues/260

606227a

tiabc approved these changes Oct 16, 2017

View reviewed changes

Oleg Raev added 7 commits October 16, 2017 21:56

imported status-go/testing for using TestConfig

1c5835b

renamed TestJailStop to TestCellsRemovedAfterSwitchAccount

e9d56a6

sync test init and test check (instead of async)

1721b1a

removed superfluous assertion

dbf19a5

Renamed TestJailStop to TestJailCellsRemovedAfterStop

15f9c79

removed WG from Jail test

b33ac59

added fixme (for rename method jailManager.Stop() in future)

f806132

tiabc reviewed Oct 16, 2017

View reviewed changes

tiabc requested review from influx6, themue and JekaMas October 16, 2017 15:35

influx6 approved these changes Oct 16, 2017

View reviewed changes

themue approved these changes Oct 16, 2017

View reviewed changes

tiabc merged commit 2401497 into status-im:develop Oct 16, 2017

tiabc mentioned this pull request Oct 16, 2017

Fix test timeouts from 1m to 5m #406

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Jail.RemoveCells before logout and switching account #382

Jail.RemoveCells before logout and switching account #382

oleg-raev commented Oct 5, 2017

divan commented Oct 5, 2017 •

edited

oleg-raev commented Oct 7, 2017 •

edited

divan commented Oct 7, 2017 •

edited

tiabc commented Oct 9, 2017 •

edited

oleg-raev commented Oct 10, 2017 •

edited

tiabc commented Oct 11, 2017

oleg-raev commented Oct 15, 2017

tiabc left a comment

tiabc Oct 16, 2017

tiabc Oct 16, 2017

tiabc Oct 16, 2017

tiabc Oct 16, 2017

tiabc Oct 16, 2017

oleg-raev Oct 16, 2017

tiabc Oct 16, 2017

tiabc Oct 16, 2017

tiabc Oct 16, 2017

tiabc Oct 16, 2017

tiabc Oct 16, 2017

oleg-raev commented Oct 16, 2017

tiabc Oct 16, 2017

influx6 left a comment

themue left a comment

tiabc commented Oct 16, 2017

tiabc commented Oct 16, 2017

Jail.RemoveCells before logout and switching account #382

Jail.RemoveCells before logout and switching account #382

Conversation

oleg-raev commented Oct 5, 2017

divan commented Oct 5, 2017 • edited

oleg-raev commented Oct 7, 2017 • edited

divan commented Oct 7, 2017 • edited

tiabc commented Oct 9, 2017 • edited

oleg-raev commented Oct 10, 2017 • edited

tiabc commented Oct 11, 2017

oleg-raev commented Oct 15, 2017

tiabc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oleg-raev commented Oct 16, 2017

Choose a reason for hiding this comment

influx6 left a comment

Choose a reason for hiding this comment

themue left a comment

Choose a reason for hiding this comment

tiabc commented Oct 16, 2017

tiabc commented Oct 16, 2017

divan commented Oct 5, 2017 •

edited

oleg-raev commented Oct 7, 2017 •

edited

divan commented Oct 7, 2017 •

edited

tiabc commented Oct 9, 2017 •

edited

oleg-raev commented Oct 10, 2017 •

edited