Skip to content

Conversation

@AurelienFT
Copy link
Contributor

@AurelienFT AurelienFT commented Feb 23, 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

The tests were failing because add_operations is managed in an other thread and so the tests don't wait that the ops are inserted before trying to fetch them. So sometimes it was fast enough sometimes not. With the new sleep it's always fast enough.

The test_pool failed after this change because it was wrong by design. Fixed it.

Fix #3509 and #3584

@AurelienFT AurelienFT requested a review from Ben-PH February 23, 2023 11:48
@AurelienFT AurelienFT changed the title Fix tests pool failing because didn't had time to add the operations … Fix tests pool failing Feb 23, 2023
@Ben-PH
Copy link
Contributor

Ben-PH commented Feb 23, 2023

LGTM. I'm gonna try something locally before I approve, but the essence of it is something like this:

for ((i = 0 ; i < 100 ; i++)); do
	cargo test --release #argument only for these tests here
done

@AurelienFT
Copy link
Contributor Author

I will fix the tests that are not related to this PR but as it's a test fixing PR let's make everything here.

@Ben-PH
Copy link
Contributor

Ben-PH commented Feb 23, 2023

I'm still getting some failures, but they are much less common:

for ((i = 0 ; i < 100 ; i++))  ; do
    cargo test test_pool -F testing --release
done

error output:

---- tests::operation_pool_tests::test_pool stdout ----
thread 'tests::operation_pool_tests::test_pool' panicked at 'assertion failed: ids.iter().map(|id|\n            (*id,\n                storage.read_operations().get(id).unwrap().serialized_data.clone())).eq(thread_tx_lists[target_slot.thread\n                            as\n                            usize].iter().filter(|(_, r)|\n                    r.contains(&target_slot.period)).take(max_count).map(|(op,\n                _)| (op.id, op.serialized_data.clone())))', massa-pool-worker/src/tests/operation_pool_tests.rs:166:21
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

improvement today is better than perfect tomorrow though. Mention this PR in the parent issue, then merge (subject to #3602 (comment))

@Ben-PH
Copy link
Contributor

Ben-PH commented Feb 23, 2023

Would it be constructive to add a comment to one of the sleeps that provides this context (not a merge-blocking suggestion)

The tests were failing because add_operations is managed in an other thread and so the tests don't wait that the ops are inserted before trying to fetch them. So sometimes it was fast enough sometimes not. With the new sleep it's always fast enough.

@AurelienFT
Copy link
Contributor Author

It shouldn't fail anymore will check on that also

@AurelienFT
Copy link
Contributor Author

@Ben-PH Can you re-review I have fixed all the tests. I don't have any fail now.

@AurelienFT AurelienFT requested a review from Ben-PH February 23, 2023 14:33
Copy link
Contributor

@Ben-PH Ben-PH left a comment

Choose a reason for hiding this comment

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

Nice. Those were bothering me, and didn't have much clue as to how to fix.

Just apply the same comment change "Allow some time for...", and for CI to finish and merge it.

@AurelienFT AurelienFT merged commit 1453cd5 into testnet_20 Feb 23, 2023
@AurelienFT AurelienFT mentioned this pull request Feb 27, 2023
bors bot added a commit that referenced this pull request Mar 1, 2023
3489: Testnet 20 r=AurelienFT a=AurelienFT

Merged in this testnet:

- #3475 
- #3549
- #3562 
- #3462 
- #3492 
- #3502 
- #3495 
- #3556 
- #3511 
- #3498 
- #3566 
- #3557 
- #3576 
- #3579 
- #3507 
- #3585 
- #3587 
- #3580 
- #3590 
- #3549 
- #3455 
- #3601 
- #3602 
- #3606 
- #3588 
- #3603 
- #3554

Co-authored-by: AurelienFT <[email protected]>
Co-authored-by: Ben PHL <[email protected]>
Co-authored-by: Modship <[email protected]>
Co-authored-by: Ben <[email protected]>
Co-authored-by: Eitu33 <[email protected]>
Co-authored-by: Sydhds <[email protected]>
Co-authored-by: modship <[email protected]>
Co-authored-by: Moncef AOUDIA <[email protected]>
Co-authored-by: Moncef AOUDIA <[email protected]>
@Ben-PH Ben-PH mentioned this pull request Mar 20, 2023
28 tasks
@AurelienFT AurelienFT deleted the fix_pool_tests_fail branch May 29, 2023 08:14
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.

3 participants