test(dot/sync): enhance tests for the worker

EmilGeorgiev commented 3 weeks ago

Describe changes

The old test doesn't guarantee that only one worker is working at a given time. For example, if someone changes the production code to:

func executeRequest(who peer.ID, requestMaker network.RequestMaker,
    task *syncTask, sharedGuard chan struct{}) {
    //defer func() {
    //  <-sharedGuard
    //}()

    if len(sharedGuard) < 1 {
        sharedGuard <- struct{}{}
    }

    //sharedGuard <- struct{}{}
        ...
}

The test will pass because len(sharedGuard) is 1.

Also, in the old test case, we ran only one worker which is not enough to guarantee that more than one worker can be running at the moment. I think it is better for at least 2 workers to be run at the same time and assert in the tests that only one of them can work at a given time.

The new implementation of the test runs two workers and checks that only one of them is active/working. At the end of the test, a new assertion that the length of the sharedGueard is 0 and the workers release the lock. In this way, we can be more confident that only one worker is running at a given time.

The new test checks and the responses that are passed to the result channel

Issues

closes: #4032

dimartiro commented 2 weeks ago

@EmilGeorgiev Thanks for your contribution. Could you please elaborate more on the PR details to describe the changes?

EmilGeorgiev commented 2 weeks ago

Hi @ EclesioMeloJunior thank you for your time and your comments. I will review and fix all of them.

EmilGeorgiev commented 2 weeks ago

All comments are fixed, and PR is updated. Also, I explained why I changed this test here: https://github.com/ChainSafe/gossamer/pull/4033#discussion_r1637928710

cc @dimartiro @EclesioMeloJunior

codecov[bot] commented 2 weeks ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 52.29%. Comparing base (d1ca7aa) to head (d8f7e3f). Report is 110 commits behind head on development.

:exclamation: Current head d8f7e3f differs from pull request most recent head 21712e9

Please upload reports for the commit 21712e9 to get more accurate results.

Additional details and impacted files

```diff @@ Coverage Diff @@ ## development #4033 +/- ## =============================================== + Coverage 50.51% 52.29% +1.78% =============================================== Files 230 248 +18 Lines 29006 25280 -3726 =============================================== - Hits 14653 13221 -1432 + Misses 12856 10464 -2392 - Partials 1497 1595 +98 ```

ChainSafe / gossamer