-
Notifications
You must be signed in to change notification settings - Fork 244
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
fix failing tests in PRs from other repos, fixes #459 #461
Conversation
geth/account/accounts_test.go
Outdated
@@ -100,12 +99,11 @@ func TestVerifyAccountPasswordWithAccountBeforeEIP55(t *testing.T) { | |||
defer os.RemoveAll(keyStoreDir) //nolint: errcheck | |||
|
|||
// Import keys and make sure one was created before EIP55 introduction. | |||
err = common.ImportTestAccount(keyStoreDir, "test-account3-before-eip55.pk") | |||
require.NoError(t, err) | |||
require.NoError(t, common.ImportTestAccount(keyStoreDir, "test-account4-public-pass.pk")) |
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 not sure if this is correct, or if I need to generate a pre-eip55 keystore for this test.
so it looks like the unit tests failed, due to the change in the genesis config. Locally I had to I'm not that familiar with travis, but it appears that the build cache is interfering. I'm not sure if that's b/c I updated this PR and opening a new PR would fix it, or if clearing the build cache is something an admin has to do. |
@perissology, the first thing, thank you for the PR! It's amazing to see how many people have got interested in contributing to Status after Devcon3.
I will do it when we decide on a solution.
You're correct, only admins can clean up Travis cache for a specific project. Regarding your solution, the PRs from forks won't execute e2e tests on the public network, so you don't need to change those tests. It's important to keep the old ones as they will have some ether to actually perform some transactions. The only thing that is left is |
I'm confused about this statement. I don't believe I changed any e2e test files. Did I?
I think this would be a fine solution, but since I also realized that I never ran the e2e tests on a private network, but are not fixed by this PR yet. It looks like there will need to be some logic in the tests that retrieves the appropriate account to use depending on the current network. For the e2e tests on the public network, we'll need to use lmk if that sounds correct and I'll implement it. |
Oh, sorry.
This is needed only for that one test Your proposed solution sounds good. Unit and e2e tests on a private network should be executed by anyone and the only solution I see is that we have two sets of accounts and some logic to select them based on the network. |
okay, I've updated the PR. I didn't dynamically generate the account for My solution was to just update the |
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.
LGTM
@@ -37,7 +37,7 @@ HELP_FUN = \ | |||
|
|||
# Main targets | |||
|
|||
UNIT_TEST_PACKAGES := $(shell go list ./... | grep -v /vendor | grep -v /e2e | grep -v /cmd) | |||
UNIT_TEST_PACKAGES := $(shell go list ./... | grep -v /vendor | grep -v /e2e | grep -v /cmd | grep -v /lib) |
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.
Thanks!
geth/common/types.go
Outdated
testConfig.Account2.Password = pass | ||
testConfig.Account3.Password = pass | ||
if networkId == params.StatusChainNetworkID { | ||
accountsData := string(static.MustAsset("config/status-chain-accounts.json")) |
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.
This is maybe better because at least consistent but just FYI if you omit conversion to string, you can omit conversion to []byte
in the next line.
@@ -6,10 +6,10 @@ | |||
"65C01586aa0Ce152835c788aCe665e91Ab3527b8": { | |||
"balance": "0x1B1AE4D6E2EF500000" | |||
}, | |||
"F35E0325dad87e2661c4eF951d58727e6d583d5c": { | |||
"bF164ca341326a03b547c05B343b2E21eFAe24b9": { |
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 wonder if two accounts above are needed anymore...?
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 looks like the 1st account is used in rpc_test.go
, but just to check that eth_getBalance
call returns > 0. This could be replaced w/ 1 of the new accounts.
The 2nd account doesn't appear to be 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.
Oh, because it's hardcoded. Let's leave it for now then.
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.
We don't need them. As I remember, that balance check used to be sort of a sanity check ensuring we have enough balance on the accounts which we use to send transactions.
testing/testing.go
Outdated
@@ -152,7 +152,7 @@ func GetHeadHashFromNetworkID(id int) string { | |||
case params.RopstenNetworkID: | |||
return "0x41941023680923e0fe4d74a34bdac8141f2540e3ae90623718e47d66d1ca4a2d" | |||
case params.StatusChainNetworkID: | |||
return "0x50e6edb4e90d9616ac8bf7119ee37c4048c41ad09328676e1b39dc68a0ecfb3d" | |||
return "0xa286ebaba3b5c6ea836d1ffdef9d7573b0f5ccdfe2ebc51fa432fb9c58fb9774" |
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.
How did you figure out that? Just by getting the first block of the chain?
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.
Just by getting the first block of the chain
essentially yes, I just let the tests do it for me...
there was a failing e2e/api
test after changing the genesis file. 0xa286
was the returned hash in the failing test.
No need, what you did looks great. The most important is that before-EIP55 account in the json file is lowercase. That's enough to test it. I run e2e tests on a private chain locally and they passed. I will try to play with Travis cache to pass it here as well. |
Hey @perissology while we wait for Travis, thanks again from the community for getting involved! Would you like to join chat.status.im? Would love to talk more about GivEth and how we can work closer together as teams in particular, and also just shoot the crypto breeze when you're not writing good code 😉 |
@andytudhope I'm already on the chat. Definitely interested in how we (Giveth) can collaborate more. I know Griff has talked to some of your team in regards to closer collaboration, but not sure of the details |
@perissology Hooray! e2e tests on a private chain passed. However, Travis also tried to run e2e tests on a public chain :( The condition in Travis should be a bit different I think. Maybe |
3bf7cd8
to
e5af63b
Compare
This commit fixes issue status-im#459 - travis -> disable e2e tests on public network for pull_requests - add 2 new test accounts to be used with unit-tests & e2e tests using StatusChain - dynamically set the TestConfig.Account* data based on the current network - exclude lib dir from UNIT_TEST_PACKAGES in Makefile
okay, sorry for spamming travis :) it looks like I got it figured out now. |
.travis.yml
Outdated
@@ -17,6 +17,7 @@ jobs: | |||
- stage: Test e2e on privite network | |||
script: make test-e2e | |||
- stage: Test e2e on public network | |||
if: type != pull_request |
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.
Another solution would be to use if: fork == false
(there is such an attribute). PRs from the origin would still run this step as the env variables will be passed.
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 did try if: fork == false
and they still ran. That build is at https://travis-ci.org/status-im/status-go/jobs/303220190/config
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.
Interesting, it looks like a bug. Let's keep type != pull_request
then 👍
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.
Add a comment justifying this line, please. It's non-obvious.
@perissology no worries :) So, I have just one final comment about using a I guess we will be fine with your current approach (using @tiabc I think this PR is good to be merged unless you have some comments :) |
testing/testing.go
Outdated
|
||
// GetAccount1PKFile returns the filename for Account1 keystore based | ||
// on the current network | ||
func GetAccount1PKFile() string { |
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 think having an intention as a comment would be more useful for understanding why it's needed.
static/config/test-data.json
Outdated
@@ -11,6 +11,7 @@ | |||
"Address": "0xA0a19221268d939c3a972bf5461dC10f7980E814" | |||
}, | |||
"Account3": { | |||
"Address": "0xa7e0214f35481b644e139d689db54b3e625f1c88" | |||
"Address": "0x3ad34e698d4806afd08b359b920f5c6b62b68ee4", | |||
"Password": "password" |
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 suppose password shouldn't be here?
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 need to store the password somewhere so geth/account/accounts_test.go
TestVerifyAccountPasswordWithAccountBeforeEIP55
can run w/o access to the ACCOUNT_PASSWORD
env variable. It seemed like the json file was a better place to store the password then hardcoding it in a function.
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.
Yeah but what I mean is that it had better be in status-chain-accounts.json
right? By following the logic that accounts in that file are supposed to be run with a hardcoded password while these should run with a password from an env variable.
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.
ah, I see. If I do that, then TestConfig.Account3
will be nil when running tests on the public chain. I don't think that will break anything, as only the unit tests use Account3
.
If that's okay, then maybe I should pull the accounts out of that file entirely. Place Account1
& Account2
in a public-chain-accounts.json
& Account3
in status-chain-accounts.json
. That feels like a cleaner solution then having some accounts in test-data.json
and then overriding those accounts when on StatusChain
.
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.
Sounds good. Please, do.
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.
Thanks for the contribution! All seems good except for a couple of minor comments.
move public testnet accounts from test-data.json to a public-chain-accounts.json file. Remove 2 unnecessary account allocations in the status-chain-genesis file
okay, I've integrated all the comments. Probably need to reset the build cache to get travis to pass as I modified the genesis file again, removing the 2 accounts mentioned above |
A short summary which serves as a squashed-commit message.
Fix failing tests for users w/o access to the
ACCOUNT_PASSWORD
env variable. And disablee2e public network
tests by travis for pull requests. Excludelib
dir from unit testsA description to understand introduced changes without reading the code.
Dynamically load the
TestConfig.Account1
&TestConfig.Account2
objects depending on if the network isStatusChain
or a public testnet.Important changes:
StatusChain
network
Solves #459