Fixing expensive tests

[MaksymZavershynskyi]

Fixes #1156

• Fixed base58->base64 encoding;
• Fixed issue with signing with wrong public key;
• WIP Sending transaction fails with RpcError "send_tx_commit has timed out."

[MaksymZavershynskyi]

Seems like the nodes just get stuck on block 1.

[bowenwang1996]

There appears to be some issue with too many open files when running all the expensive tests together. I think it might be related to actix, but don't have time right now to look deeply into this.

[MaksymZavershynskyi]

Actix opens files? Maybe it is somehow related to actix opening too many sockets me and @evgenykuzyakov can see on our machines that Actix opens ~64k sockets (based on logs). Socket is a file descriptor so maybe that's confusing. Independently on it, expensive tests are really important, because none of the tests in near/tests try submitting transactions.

[MaksymZavershynskyi]

Btw, expensive tests do not all run together. There is a special macro heavy_test that prevents them from starting at the same time using Mutex and ShutdownableThread makes sure previous tests shuts down before the next one starts.

But also, you can run tests individually and see that they do not pass.

[bowenwang1996]

Yes I am aware that heavy_test exists. But they still fail when run together for some reason. Which test fail when you run it separately?

[MaksymZavershynskyi]

Yes I am aware that heavy_test exists. But they still fail when run together for some reason. Which test fail when you run it separately?

I haven not recorded it. Do they all pass for you when run separately?

[bowenwang1996]

I didn't run all of them (it would take a while), but from about 10 tests I run, all of them pass. I can run all of them if that's desired. But I think it would be helpful if we can fix this problem of too many open files and do not have to run 40 tests manually.

[MaksymZavershynskyi]

Okay, now that I pull the most recent commit from this branch I cannot find a test that fails when run individually. However, it would be nice to make it work, because they are otherwise useless. No one is going to run 30 tests by hand, it is equal to not having them.

[bowenwang1996]

While trying to fix the expensive tests after merging with staging, I noticed that the ones that involve running nodes have different behavior than the runtime only tests due to storage rent. Since the behaviors diverge, we cannot use the same code for both runtime unit tests and integration tests. So we should consider either separating the tests (breaking the existing test framework) or relax the conditions. @evgenykuzyakov @nearmax

[ilblackdragon]

The behavior diverges, because in runtime tests, some tests specify the Genesis config properties around rent. We have two options: - also just specify those options in non runtime tests (e.g copy that setting we have with expensive storage into the other test) - move Genesis configuration be some part of standard test cases info, like a each test specifies which Genesis scenario it needs.

…

On Thu, Aug 22, 2019, 1:19 AM Bowen Wang ***@***.***> wrote: While trying to fix the expensive tests after merging with staging, I noticed that the ones that involve running nodes have different behavior than the runtime only tests due to storage rent. Since the behaviors diverge, we cannot use the same code for both runtime unit tests and integration tests. So we should consider either separating the tests (breaking the existing test framework) or relax the conditions. @evgenykuzyakov <https://github.com/evgenykuzyakov> @nearmax <https://github.com/nearmax> — You are receiving this because you were assigned. Reply to this email directly, view it on GitHub <#1157>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABK27SOJIQYLXTVWTAY5B3QFXERXANCNFSM4IK44C6A> .

[ilblackdragon]

Right now still a bunch of tests with fail in staging:

cargo test -p nearcore --all-features

failures:
test::test_access_key_smart_contract_testnet
test::test_add_access_key_function_call_testnet
test::test_add_access_key_with_allowance_testnet
test::test_add_existing_key_testnet
test::test_add_key_testnet
test::test_create_account_again_testnet
test::test_create_account_failure_already_exists_testnet
test::test_create_account_failure_invalid_name_testnet
test::test_create_account_testnet
test::test_delete_access_key_testnet
test::test_delete_access_key_with_allowance_testnet
test::test_delete_key_last_testnet
test::test_delete_key_not_owned_testnet
test::test_delete_key_testnet
test::test_nonce_update_when_deploying_contract_testnet
test::test_redeploy_contract_testnet
test::test_refund_on_send_money_to_non_existent_account_testnet
test::test_send_money_testnet
test::test_smart_contract_bad_method_name_testnet
test::test_smart_contract_empty_method_name_with_no_tokens_testnet
test::test_smart_contract_self_call_testnet
test::test_smart_contract_simple_testnet
test::test_smart_contract_with_args_testnet

[MaksymZavershynskyi]

@SkidanovAlex @bowenwang1996 Is this an abandoned PR? Should it be closed?

[bowenwang1996]

I think this should still be fixed unless we don't want to run those tests any more.

[ilblackdragon]

There might be still some file descriptor leak, but on my laptop with 10k ulimit all --p nearcore tests pass. Let's merge this @nearmax @SkidanovAlex to get make nightly greener.

[codecov]

Codecov Report

Merging #1157 into staging will decrease coverage by 2.79%.
The diff coverage is 71.42%.

@@            Coverage Diff             @@
##           staging    #1157     +/-   ##
==========================================
- Coverage    81.21%   78.42%   -2.8%     
==========================================
  Files          169      169             
  Lines        37905    40633   +2728     
==========================================
+ Hits         30785    31865   +1080     
- Misses        7120     8768   +1648

Impacted Files	Coverage Δ
test-utils/testlib/src/standard_test_cases.rs	98.74% <ø> (-0.06%)	⬇️
test-utils/testlib/src/user/runtime_user.rs	88.33% <ø> (ø)	⬆️
runtime/runtime/src/lib.rs	92.7% <ø> (+1.01%)	⬆️
test-utils/testlib/src/node/mod.rs	100% <ø> (ø)	⬆️
runtime/near-vm-logic/tests/helpers.rs	97.82% <ø> (ø)	⬆️
runtime/near-vm-logic/tests/test_miscs.rs	100% <ø> (ø)	⬆️
chain/jsonrpc/client/src/message.rs	83.27% <0%> (+0.07%)	⬆️
near/src/config.rs	61.28% <0%> (+2.48%)	⬆️
chain/client/src/client_actor.rs	68.54% <100%> (+0.71%)	⬆️
tests/test_errors.rs	100% <100%> (+1.42%)	⬆️
... and 77 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f84cf19...bbf9d3e. Read the comment docs.

[MaksymZavershynskyi]

There might be still some file descriptor leak, but on my laptop with 10k ulimit all --p nearcore tests pass. Let's merge this @nearmax @SkidanovAlex to get make nightly greener.

Sure. I cannot approve it since it is on my name. Also, the CI seems to be failing.