test(source): remove flaky log assertions from pod tests

[u-kai]

What does it do ?

Remove flaky log assertions from TestPodSource to fix race conditions that caused intermittent test failures with "Expected no debug messages" errors.

Example of the race condition failure: : link

Motivation

The TestPodSource test was experiencing flaky failures due to race conditions when running in parallel.
The issue occurred because:

1. Global logger state conflicts: Multiple tests with t.Parallel() were simultaneously
   modifying the global logrus logger through LogsUnderTestWithLogLevel()
2. Implementation detail testing: Testing log output (an implementation detail) rather than
   focusing on the core functionality

Solution: Remove log testing entirely and focus on endpoint generation correctness, which is
the actual functionality being tested.

More

• Yes, this PR title follows Conventional Commits
• Yes, I added unit tests
• Yes, I updated end user documentation accordingly

[k8s-ci-robot]

Hi @u-kai. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

[mloiseleur]

/ok-to-test

[ivankatliarchuk]

If you removing this capability from tests, we need a compensation. Testing similar capabilities but without paralel execution

[u-kai]

@ivankatliarchuk
Thank you for review!

I personally don't think the log assertions in this test are essential, especially since it's just Debug logging.
I prioritized enabling t.Parallel() to speed up the test suite, and I believe removing the log assertions makes the test more robust since log output tends to change frequently.
Could you clarify why you think it's important to keep testing the logs in this case?

[ivankatliarchuk]

/hold

[ivankatliarchuk]

compensation for removed testing capability is required.

[ivankatliarchuk]

as well as static analysis for correct use of test.Parallel() is needed

[u-kai]

@ivankatliarchuk

compensation for removed testing capability is required.

The only part that is no longer being tested after my change is the log output.
As I understand it, your suggestion for a compensation would mean explicitly verifying that the expected log messages are produced — is that correct?
To my knowledge, very few of the existing tests currently verify that the log output is correct, so I just want to make sure I am interpreting your request properly.
Could you please confirm whether this understanding is correct?

[ivankatliarchuk]

Yes, there is no need to validate same logs for each test, so only few tests have logging valided. We could have separate test that just verify log messages. Agree that it does single log level testing, not efficient.

[u-kai]

@ivankatliarchuk
Thanks for the helpful advice! I've added a separate test for log verification as you suggested.
It would be great if you could take another look when you have a chance.

[ivankatliarchuk]

/lgtm

[ivankatliarchuk]

do we need Parallel for that? Just a comment, not necessary need to action

[u-kai]

I think it's needed here, because as discussed below, using t.Parallel at this level is safe and allows us to parallelize the tests.

[ivankatliarchuk]

/unhold

[mloiseleur]

See my suggestion: t.Parallel() should be inside the for loop.
apart from that, LGTM. Thanks for your help on this 👍

[u-kai]

@mloiseleur
Thanks for your review.
I applied your suggestion and ran the tests multiple times. However, I noticed that the test sometimes fails like this:

--- FAIL: TestPodSourceLogs (0.00s)
    --- FAIL: TestPodSourceLogs/when_ignoreNonHostNetworkPods=false,_no_skip_logs_should_be_generated (0.10s)
        pod_test.go:895:
                Error Trace:    /Users/kai/external-dns/internal/testutils/log.go:91
                Error:          Should be false
                Test:           TestPodSourceLogs/when_ignoreNonHostNetworkPods=false,_no_skip_logs_should_be_generated
                Messages:       Expected log message found when should not: skipping pod my-pod1-65699. hostNetwork=false
FAIL

This happens because the parallel tests share state and have conflicting expectations about whether certain log messages should or should not appear.
While this can be avoided by making sure test data (like variable names) don’t overlap between tests, this requires extra care and could easily introduce mistakes in the future.

For this reason, I personally prefer keeping the current approach without using t.Parallel() here — what do you think?

[mloiseleur]

For this reason, I personally prefer keeping the current approach without using t.Parallel() here — what do you think?

Make sense to me. We want the CI to be as reliable as we can 👍

[u-kai]

Make sense to me. We want the CI to be as reliable as we can 👍

Got it, I'll leave it as is then.

[mloiseleur]

/approve

[ivankatliarchuk]

/test pull-external-dns-unit-test

[ivankatliarchuk]

/test all

[ivankatliarchuk]

/lgtm

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ivankatliarchuk, mloiseleur

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

• OWNERS [ivankatliarchuk,mloiseleur]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment