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

Tests: PR for #2067 #2068

Merged
merged 1 commit into from
Nov 26, 2019
Merged

Tests: PR for #2067 #2068

merged 1 commit into from
Nov 26, 2019

Conversation

MichelSantos
Copy link
Contributor

@MichelSantos MichelSantos commented Nov 23, 2019

This is a hack to resolve #2067

@pmconrad
Copy link
Contributor

Is there any obvious reason why the two test cases you fixed do not use cli_fixture?
I find it strange that the problem is caused by these two tests only, not by those that use cli_fixture (which also simply calls application::shutdown in its destructor).

@MichelSantos
Copy link
Contributor Author

Is there any obvious reason why the two test cases you fixed do not use cli_fixture?

It's not obvious to me. I think that they could be converted to use it.

I find it strange that the problem is caused by these two tests only, not by those that use cli_fixture (which also simply calls application::shutdown in its destructor).

I have concluded that the problem is that these two tests (cli_multisig_transaction and cli_create_htlc) interfere with the next tests (saving_keys_wallet_test and cli_sign_message) in the following way:

  1. The prior tests create an graphene::app::application directly rather than indirectly create through a cli_fixture

  2. The prior tests invoke app1->shutdown() and then immediately exit. In contrast, the destructor of a cli_fixture invokes app1->shutdown() and then delays due to the cli_fixture dummy destructor. (It is interesting that this delaying was introduced in PR Add sleep to cli_test #1626 as a fix for cli_tests memory access violation at address: no mapping at fault address #1303)

  3. The beginning of the next tests construct a cli_fixture. If this construction is initiated too soon (on the order of 100 ms on the Travis machines) after the prior test's call to app1->shutdown then Travis encounters the memory reference problem.

I suspect that there is an undesired interaction between the two tests either through the OS file system or the OS networking.

The approach taken in this PR is to heuristicallly clean up the prior environment (cli_multisig_transaction and cli_create_htlc) with a delay at the conclusion of those tests.

One alternative is to heuristically clean up the environment in the subsequent tests (saving_keys_wallet_test and cli_sign_message) with a delay at the beginning of those tests.

The best alternative, in my opinion, is to explicitly delay and affirm that the environment is clean before exiting each test (these two tests and those using cli_fixture). But this will require explicitly identifying the direct mechanism of interaction between tests to subsequently be able to check. It might be low enough priority that the heuristic approach taken here is adequate.

@abitmore
Copy link
Member

FWIW the underlying reason is described in OP of #1784.

@oxarbitrage oxarbitrage merged commit de7d43f into bitshares:release Nov 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants