Bugfix: Fix Failing UT missing credential sanitizer

[yashnap]

Similar Issue and Fix : #374

Its weird because I see failure in one of my PR run : #368

and the 2nd one looks fine after the same rebase : #359

The addition of credential sanitizer has started exercising new paths which is reducing the coverage from base branch.

• I've removed the test that were marked to skip on GitHub because they kept failing according to this PR : Fix failing GitHub workflow tests #129 . ( No comment was mentioned about putting it back or reasoning either)
• Added sleep time 20s and 30s each for update time use-case which was failing on assertion due to time not getting reflected or writes happening before/around the same time.
  Dont see any failures at the moment with the github tests that were failing earlier.

[Copilot]

Pull request overview

Updates extension unit tests to align with the TelemetryWriter constructor now requiring a credential_sanitizer, resolving UT failures caused by missing parameter injection.

Changes:

• Pass self.runtime.credential_sanitizer when re-instantiating TelemetryWriter in Test_ActionHandler.
• Remove an extraneous trailing semicolon in a test statement.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 94.87%. Comparing base (ed52180) to head (5db37ff).
⚠️ Report is 1 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #376      +/-   ##
==========================================
+ Coverage   94.69%   94.87%   +0.18%     
==========================================
  Files         111      111              
  Lines       20860    20855       -5     
==========================================
+ Hits        19753    19787      +34     
+ Misses       1107     1068      -39     

Flag	Coverage Δ
python27	94.87% <100.00%> (+0.26%)	⬆️
python312	94.87% <100.00%> (+0.18%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[Copilot]

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

Suppressed comments (1)

src/extension/tests/Test_ActionHandler.py:314

• Remove the stray "#test" comment; it looks like leftover debug noise and adds no value to the test.

        os.getenv = mock_os_getenv
        #test
        self.runtime.telemetry_writer = TelemetryWriter(self.runtime.logger, self.runtime.env_layer, self.runtime.credential_sanitizer)

[Copilot]

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

[yashnap]

Project Codecov failure:
Suspicion is that due to multiple PR's getting merged which touched similar files( Dnf5 related) and new path, dropped the overall coverage for the base branch.

Current PR coverage :

My other PR will be bridging the code coverage gap which is targeting DNF5 related files: #368

We can merge the current PR to avoid failures in UT.

Bit weird why the UT failures didnt show up on the actual PR of mine that caused this issue,

[Copilot]

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

[Copilot]

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

Suppressed comments (2)

src/extension/tests/Test_TelemetryWriter.py:42

• test_write_event is being re-enabled on GitHub runners even though this file already documents GitHub-runner-specific flakiness/failures (e.g., assertions marked "Fails here on GitHub"). With the early-return removed, CI on GitHub Actions is likely to regress/flap again. If the test isn’t being stabilized in this PR, it should be explicitly skipped on GitHub runners (prefer skipTest() over return so it’s reported as skipped).

        # if self.runtime.is_github_runner:
        #     return

src/extension/tests/Test_ExtOutputStatusHandler.py:95

• The added time.sleep(0.02) delays won’t actually run in these tests because RuntimeComposer monkeypatches time.sleep to a no-op globally (see extension/tests/helpers/RuntimeComposer.py). That means this test will remain flaky on filesystems with coarse mtime granularity. Instead of sleeping, force the file’s mtime to a known value before the update and assert it changes after the write (no timing dependency).

        time.sleep(0.02)
        ext_status_handler.update_file("test1")
        stat_file_name = os.stat(os.path.join(dir_path, file_name + ".status"))
        modified_time = stat_file_name.st_mtime
        self.assertEqual(prev_modified_time, modified_time)

        time.sleep(0.02)  # ensure filesystem mtime granularity is exceeded
        ext_status_handler.update_file(file_name)

[Copilot]

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

Suppressed comments (2)

src/extension/tests/Test_ExtOutputStatusHandler.py:86

• test_update_file uses time.sleep() + filesystem st_mtime comparisons to detect updates. This is brittle on CI (mtime granularity varies) and, in these tests, RuntimeComposer monkey-patches time.sleep to a no-op, so the added sleeps don't reliably change anything. You can make the test deterministic by asserting the status JSON remains unchanged for the wrong seq_no and changes for the correct seq_no, without relying on mtime.

        stat_file_name = os.stat(os.path.join(dir_path, file_name + ".status"))
        prev_modified_time = stat_file_name.st_mtime

        time.sleep(0.02)
        ext_status_handler.update_file("test1")

src/extension/tests/Test_TelemetryWriter.py:43

• test_write_event is still timing- and os.listdir()-order dependent. Now that the GitHub-runner skip is removed, the test can be flaky because it assumes either (a) two files and that index [1] corresponds to the second write, or (b) one file containing exactly two events. A deterministic approach is to read and aggregate events across all event files (sorted) and assert the expected TaskNames/events were written.

        self.telemetry_writer.write_event("testing telemetry write to file", Constants.TelemetryEventLevel.Error, "Test Task")
        with open(os.path.join(self.telemetry_writer.events_folder_path, os.listdir(self.telemetry_writer.events_folder_path)[0]), 'r+') as f:
            events = json.load(f)

[Copilot]

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

Suppressed comments (3)

src/extension/tests/Test_ExtOutputStatusHandler.py:86

• The new time.sleep(...) calls don’t reliably fix the GitHub runner failure because RuntimeComposer replaces time.sleep with a no-op in setUp (RuntimeComposer.py:25-34). Also, asserting st_mtime changes is inherently filesystem-dependent and can be 1-second granular. This test already validates the real behavior via the JSON content assertions below—remove the mtime/sleep checks and instead assert that updating a non-existent seq doesn’t create a new status file.

        stat_file_name = os.stat(os.path.join(dir_path, file_name + ".status"))
        prev_modified_time = stat_file_name.st_mtime

        time.sleep(0.02)
        ext_status_handler.update_file("test1")

src/extension/tests/Test_TelemetryWriter.py:44

• Now that the GitHub-runner skip is removed, test_write_event is still nondeterministic: it relies on os.listdir(...)[0] / [1] ordering and on whether the two writes land in one file or two (both are timing/FS dependent). The test itself notes GitHub failures. Make it deterministic by collecting all events across all event files and asserting the set of TaskNames, independent of file count and ordering.

    def test_write_event(self):
        self.telemetry_writer.write_event("testing telemetry write to file", Constants.TelemetryEventLevel.Error, "Test Task")
        with open(os.path.join(self.telemetry_writer.events_folder_path, os.listdir(self.telemetry_writer.events_folder_path)[0]), 'r+') as f:
            events = json.load(f)
            self.assertTrue(events is not None)

src/extension/tests/Test_ExtOutputStatusHandler.py:22

• import time was added but is only used for the mtime-granularity sleeps in test_update_file. Since RuntimeComposer monkeypatches time.sleep to a no-op (RuntimeComposer.py:25-34), those sleeps don’t actually wait. If you remove the mtime-based assertions (see next comment), time becomes unused and should be removed to avoid dead imports.

This issue also appears on line 82 of the same file.

import json
import os
import shutil
import tempfile
import time
import unittest

[Copilot]

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

Suppressed comments (3)

src/extension/tests/Test_ExtOutputStatusHandler.py:95

• test_update_file adds time.sleep(...) to try to exceed filesystem mtime granularity, but RuntimeComposer globally replaces time.sleep with a no-op (time.sleep = self.mock_sleep). These sleeps never wait, so the mtime-based assertions remain flaky. Since the test already validates update_file behavior via JSON content checks, drop the mtime assertions (and the sleeps) to make the test deterministic.

        time.sleep(0.02)
        ext_status_handler.update_file("test1")
        stat_file_name = os.stat(os.path.join(dir_path, file_name + ".status"))
        modified_time = stat_file_name.st_mtime
        self.assertEqual(prev_modified_time, modified_time)

        time.sleep(0.03)  # ensure filesystem mtime granularity is exceeded
        ext_status_handler.update_file(file_name)
        stat_file_name = os.stat(os.path.join(dir_path, file_name + ".status"))
        modified_time = stat_file_name.st_mtime
        self.assertNotEqual(prev_modified_time, modified_time)  # Fails here on GitHub

src/extension/tests/Test_TelemetryWriter.py:57

• test_write_event indexes directly into os.listdir(...) (e.g., [0], [1]). os.listdir order is not guaranteed, so this can read the wrong event file on some filesystems/runners (which matches the existing "Fails here on GitHub" note). Now that the GitHub-runner skip was removed, this needs to be made deterministic (e.g., sort filenames and assert against the last event in the newest file).

This issue also appears on line 111 of the same file.

        self.telemetry_writer.write_event("testing telemetry write to file", Constants.TelemetryEventLevel.Error, "Test Task")
        with open(os.path.join(self.telemetry_writer.events_folder_path, os.listdir(self.telemetry_writer.events_folder_path)[0]), 'r+') as f:
            events = json.load(f)
            self.assertTrue(events is not None)
            self.assertEqual(events[0]["TaskName"], "Test Task")
            f.close()

        self.telemetry_writer.write_event("testing telemetry write to file", Constants.TelemetryEventLevel.Error, "Test Task2")
        if len(os.listdir(self.telemetry_writer.events_folder_path)) > 1:
            with open(os.path.join(self.telemetry_writer.events_folder_path, os.listdir(self.telemetry_writer.events_folder_path)[1]), 'r+') as f:
                events = json.load(f)
                self.assertTrue(events is not None)
                self.assertEqual(events[0]["TaskName"], "Test Task2")
                f.close()
        else:
            with open(os.path.join(self.telemetry_writer.events_folder_path, os.listdir(self.telemetry_writer.events_folder_path)[0]), 'r+') as f:
                events = json.load(f)

src/extension/tests/Test_TelemetryWriter.py:116

• test_delete_older_events relies on os.listdir(...) ordering (old_events[0] not in new_events), but os.listdir order is not guaranteed. This makes the assertion nondeterministic and matches the existing "Fails here on GitHub" note. Now that the GitHub-runner skip is removed, sort the filenames (they’re timestamp-based) and/or assert via set difference instead of using index 0.

    def test_delete_older_events(self):
        # deleting older event files before adding new one
        self.telemetry_writer.write_event("testing telemetry write to file", Constants.TelemetryEventLevel.Error, "Test Task")
        self.telemetry_writer.write_event("testing telemetry write to file", Constants.TelemetryEventLevel.Error, "Test Task2")
        self.telemetry_writer.write_event("testing telemetry write to file", Constants.TelemetryEventLevel.Error, "Test Task3")
        old_events = os.listdir(self.telemetry_writer.events_folder_path)

[Copilot]

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

Suppressed comments (4)

src/extension/tests/Test_ExtOutputStatusHandler.py:86

• The new time.sleep() calls won’t actually delay in this test suite because RuntimeComposer globally overrides time.sleep with a no-op (tests/helpers/RuntimeComposer.py). That means the mtime-based assertions here will remain flaky/failing on fast filesystems/1s mtime granularity. Consider removing the sleeps/mtime checks and instead asserting that update_file('test1') leaves the status JSON unchanged, while update_file(file_name) updates the status fields as expected (which you already validate below).

        stat_file_name = os.stat(os.path.join(dir_path, file_name + ".status"))
        prev_modified_time = stat_file_name.st_mtime

        time.sleep(0.02)
        ext_status_handler.update_file("test1")

src/extension/tests/Test_TelemetryWriter.py:44

• By removing the GitHub-runner skip, this test now runs in CI, but it still depends on nondeterministic os.listdir() ordering and has a branch with a known-failing assertion ("Fails here on GitHub"). To make it stable, avoid assuming whether events land in 1 vs N files; instead, read all event files and assert both events were written.

This issue also appears on line 111 of the same file.

    def test_write_event(self):
        self.telemetry_writer.write_event("testing telemetry write to file", Constants.TelemetryEventLevel.Error, "Test Task")
        with open(os.path.join(self.telemetry_writer.events_folder_path, os.listdir(self.telemetry_writer.events_folder_path)[0]), 'r+') as f:
            events = json.load(f)
            self.assertTrue(events is not None)

src/extension/tests/Test_TelemetryWriter.py:116

• old_events[0] relies on os.listdir() order (not guaranteed) and can miss regressions if the surviving/new file name happens to not be the first element. Since the test already expects only 1 file after cleanup, assert that none of the pre-existing filenames remain.

    def test_delete_older_events(self):
        # deleting older event files before adding new one
        self.telemetry_writer.write_event("testing telemetry write to file", Constants.TelemetryEventLevel.Error, "Test Task")
        self.telemetry_writer.write_event("testing telemetry write to file", Constants.TelemetryEventLevel.Error, "Test Task2")
        self.telemetry_writer.write_event("testing telemetry write to file", Constants.TelemetryEventLevel.Error, "Test Task3")
        old_events = os.listdir(self.telemetry_writer.events_folder_path)

src/extension/tests/Test_ExtOutputStatusHandler.py:22

• time is imported but only used for the time.sleep() calls above. If you remove the sleeps (or otherwise stop using time here), please drop this import to avoid an unused dependency in the test file.

This issue also appears on line 82 of the same file.

import os
import shutil
import tempfile
import time
import unittest

[Copilot]

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

Suppressed comments (3)

src/extension/tests/Test_TelemetryWriter.py:126

• old_events[0] depends on os.listdir() ordering, so this assertion can fail even when the deletion behavior is correct. Prefer a deterministic assertion (e.g., compare sets) so the test doesn't depend on directory ordering.

        new_events = os.listdir(self.telemetry_writer.events_folder_path)
        self.assertEqual(len(new_events), 1)
        self.assertNotIn(old_events[0], new_events)
        Constants.TELEMETRY_DIR_SIZE_LIMIT_IN_CHARS = telemetry_dir_size_backup

src/extension/tests/Test_ExtOutputStatusHandler.py:86

• The test relies on filesystem mtime changes with sub-100ms sleeps. On many CI filesystems (incl. some GitHub runners) mtime granularity is 1s, so this assertion can remain flaky even after adding sleeps. Consider asserting behavior via file existence/content instead (e.g., ensure update_file("test1") does not create a new status file, and validate the JSON content changes after update_file(file_name)) rather than comparing st_mtime.

        stat_file_name = os.stat(os.path.join(dir_path, file_name + ".status"))
        prev_modified_time = stat_file_name.st_mtime

        time.sleep(0.02)
        ext_status_handler.update_file("test1")

src/extension/tests/Test_TelemetryWriter.py:43

• This test reads the “first” event file via os.listdir()[0]/[1], but directory iteration order is not guaranteed. After removing the GitHub-runner skip, this can reintroduce nondeterministic failures. Sort the filenames (or select by mtime) before indexing so the test consistently reads the intended event file(s).

This issue also appears on line 123 of the same file.

        self.telemetry_writer.write_event("testing telemetry write to file", Constants.TelemetryEventLevel.Error, "Test Task")
        with open(os.path.join(self.telemetry_writer.events_folder_path, os.listdir(self.telemetry_writer.events_folder_path)[0]), 'r+') as f:
            events = json.load(f)