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

Reinstate pool tests #3486

Merged
merged 8 commits into from
Feb 7, 2023
Merged

Reinstate pool tests #3486

merged 8 commits into from
Feb 7, 2023

Conversation

Ben-PH
Copy link
Contributor

@Ben-PH Ben-PH commented Feb 1, 2023

  • document all added functions
  • try in sandbox /simulation/labnet
  • unit tests on the added/changed features
    • make tests compile
    • make tests pass
  • add logs allowing easy debugging in case the changes caused problems
  • if the API has changed, update the API specification

@Ben-PH Ben-PH marked this pull request as draft February 1, 2023 13:01
@Ben-PH Ben-PH mentioned this pull request Feb 1, 2023
@Ben-PH Ben-PH changed the title Benph/reinstate pool tests Reinstate pool tests Feb 1, 2023
@Ben-PH
Copy link
Contributor Author

Ben-PH commented Feb 1, 2023

still need to refactor operation_pool_tests.rs::test_pool to use pool_test wrapper

EDIT: might not end up using it...

@AurelienFT AurelienFT changed the base branch from testnet_19 to testnet_20 February 1, 2023 15:01
@Ben-PH
Copy link
Contributor Author

Ben-PH commented Feb 1, 2023

67d2921 adds some dbg!s, and this is the typical output:

(will get through a few threads from time to time...)

     Running unittests src/lib.rs (target/debug/deps/massa_pool_worker-c92c862378c0b4ac)

running 1 test
[massa-pool-worker/src/tests/operation_pool_tests.rs:129] thread = 0
[massa-pool-worker/src/operation_pool.rs:224] &slot = Slot {
    period: 31,
    thread: 0,
}
[massa-execution-exports/src/test_exports/mock.rs:217] "never seen this not err..." = "never seen this not err..."
[massa-execution-exports/src/test_exports/mock.rs:218] response_rx.recv_timeout(Duration::from_millis(100)) = Err(
    Timeout,
)
thread 'tests::operation_pool_tests::test_pool' panicked at 'called `Result::unwrap()` on an `Err` value: Timeout', massa-execution-exports/src/test_exports/mock.rs:218:68

Still need to work out the source of the problem...

@Ben-PH Ben-PH force-pushed the benph/reinstate-pool-tests branch from 67d2921 to a3fb651 Compare February 3, 2023 10:48
@Ben-PH
Copy link
Contributor Author

Ben-PH commented Feb 3, 2023

At this point, two of the methods are unignored, but the third one is not clear for me yet. @AurelienFT what do you think of merging these two so far, and do the rest in a follow up PR. It at least makes these two available in the meantime.

I think it comes down to the balance comparison
@Ben-PH
Copy link
Contributor Author

Ben-PH commented Feb 3, 2023

Turns out needed to forward on the messages, and provide a balance that allowed the operations to occur.

Still has an issue with storage cleanup.

@Ben-PH
Copy link
Contributor Author

Ben-PH commented Feb 3, 2023

test is flakey: run this inside massa-pool-worker to (eventually) get a fail;

while cargo test test_pool; do :; done

@AurelienFT
Copy link
Contributor

I didn't follow well everything that has been going on but to give you maybe some context that might help you. We always run the test at root with feature testing (to have exports only when test cfg (no other way with cargo yet)).

@Ben-PH
Copy link
Contributor Author

Ben-PH commented Feb 6, 2023

updated script:

COUNTER = 0;
while cargo test test_pool -F testing --release ; do
    COUNTER=$[$COUNTER +1]
    :
done

echo $COUNTER;

@Eitu33 had a look at it with me, and it seems that this comes down to concurrent nature of the way the tests are setup?

The means to fix it is similar to the other two tests: Use the mock-receiver to intercept requests for data, and reply with hardcoded/intercepted data.

With that in mind, I'll do the final tidy up then mark for review.

@@ -189,7 +212,7 @@ fn test_pool() {
let fee = 1000;
let expire_period: u64 = 300;
let op = get_transaction(expire_period, fee);
let mut storage = Storage::create_root();
let mut storage = storage_base.clone_without_refs();
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was it the case in the past that you could create multiple instances of storage without panic!ing? That could explain the need for this change.

@Ben-PH Ben-PH requested review from AurelienFT February 6, 2023 15:34
@Ben-PH Ben-PH marked this pull request as ready for review February 6, 2023 15:34
@Ben-PH Ben-PH requested a review from Eitu33 February 6, 2023 15:35
@Ben-PH Ben-PH linked an issue Feb 6, 2023 that may be closed by this pull request
@Eitu33 Eitu33 merged commit ea59909 into testnet_20 Feb 7, 2023
@AurelienFT AurelienFT deleted the benph/reinstate-pool-tests branch May 29, 2023 08:15
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.

Repair pool tests
3 participants