Skip to content

Conversation

pippolo84
Copy link
Member

TestWaitForCacheSyncWithTimeout is relying on subtests, so a new assert object should be derived from each subtest testing.T, in order to report the failing one correctly.

Temporarily changing the code to force a failure in the Not invoking BlockWaitGroupToSyncResources should cause wait to succeed immediately case, it can be seen that no subtest is reported as failing:

--- FAIL: TestWaitForCacheSyncWithTimeout (0.00s)
    --- PASS: TestWaitForCacheSyncWithTimeout/Waiting_for_no_resources_should_always_sync (0.00s)
    --- PASS: TestWaitForCacheSyncWithTimeout/Not_invoking_BlockWaitGroupToSyncResources_should_cause_wait_to_succeed_immediately (0.00s)
    --- PASS: TestWaitForCacheSyncWithTimeout/Should_timeout_due_to_watched_resource_exceeding_timeout (0.20s)
    --- PASS: TestWaitForCacheSyncWithTimeout/Any_one_timeout_should_cause_error (0.60s)
    --- PASS: TestWaitForCacheSyncWithTimeout/Should_complete_due_to_event_causing_timeout_to_be_extended_past_initial_timeout (0.70s)

While after this change the expected subtest is correctly reported as the offending one:

--- FAIL: TestWaitForCacheSyncWithTimeout (0.00s)
    --- PASS: TestWaitForCacheSyncWithTimeout/Waiting_for_no_resources_should_always_sync (0.00s)
    --- FAIL: TestWaitForCacheSyncWithTimeout/Not_invoking_BlockWaitGroupToSyncResources_should_cause_wait_to_succeed_immediately (0.00s)
    --- PASS: TestWaitForCacheSyncWithTimeout/Should_timeout_due_to_watched_resource_exceeding_timeout (0.20s)
    --- PASS: TestWaitForCacheSyncWithTimeout/Any_one_timeout_should_cause_error (0.60s)
    --- PASS: TestWaitForCacheSyncWithTimeout/Should_complete_due_to_event_causing_timeout_to_be_extended_past_initial_timeout (0.70s)

TestWaitForCacheSyncWithTimeout is relying on subtests, so a new assert
object should be derived from each subtest testing.T, in order to report
the failing one correctly.

Temporarily changing the code to force a failure in the "Not invoking
BlockWaitGroupToSyncResources should cause wait to succeed immediately"
case, it can be seen that no subtest is reported as failing:

--- FAIL: TestWaitForCacheSyncWithTimeout (0.00s)
    --- PASS: TestWaitForCacheSyncWithTimeout/Waiting_for_no_resources_should_always_sync (0.00s)
    --- PASS: TestWaitForCacheSyncWithTimeout/Not_invoking_BlockWaitGroupToSyncResources_should_cause_wait_to_succeed_immediately (0.00s)
    --- PASS: TestWaitForCacheSyncWithTimeout/Should_timeout_due_to_watched_resource_exceeding_timeout (0.20s)
    --- PASS: TestWaitForCacheSyncWithTimeout/Any_one_timeout_should_cause_error (0.60s)
    --- PASS: TestWaitForCacheSyncWithTimeout/Should_complete_due_to_event_causing_timeout_to_be_extended_past_initial_timeout (0.70s)

While after this change the expected subtest is correctly reported as the
offending one:

--- FAIL: TestWaitForCacheSyncWithTimeout (0.00s)
    --- PASS: TestWaitForCacheSyncWithTimeout/Waiting_for_no_resources_should_always_sync (0.00s)
    --- FAIL: TestWaitForCacheSyncWithTimeout/Not_invoking_BlockWaitGroupToSyncResources_should_cause_wait_to_succeed_immediately (0.00s)
    --- PASS: TestWaitForCacheSyncWithTimeout/Should_timeout_due_to_watched_resource_exceeding_timeout (0.20s)
    --- PASS: TestWaitForCacheSyncWithTimeout/Any_one_timeout_should_cause_error (0.60s)
    --- PASS: TestWaitForCacheSyncWithTimeout/Should_complete_due_to_event_causing_timeout_to_be_extended_past_initial_timeout (0.70s)

Signed-off-by: Fabio Falzoi <fabio.falzoi@isovalent.com>
@pippolo84 pippolo84 added kind/bug/CI This is a bug in the testing code. release-note/misc This PR makes changes that have no direct user impact. labels Jun 13, 2024
@pippolo84 pippolo84 requested a review from a team as a code owner June 13, 2024 14:26
@pippolo84 pippolo84 requested a review from nathanjsweet June 13, 2024 14:26
@pippolo84
Copy link
Member Author

/test

@maintainer-s-little-helper maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Jun 14, 2024
@borkmann borkmann merged commit cf63069 into cilium:main Jun 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug/CI This is a bug in the testing code. ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/misc This PR makes changes that have no direct user impact.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants