test: add missing analyzer unit tests and datafinder date validation by SNO7E-G · Pull Request #3783 · google/timesketch

SNO7E-G · 2026-03-25T18:17:10Z

Hi Team 👋
This PR cleans up four `TODO` comments that have been sitting in the codebase for
a while — all of them asking for test coverage or input validation that was never
implemented. Nothing drastic, just filling in the gaps.
Closes #3782

What's been bothering me (and hopefully you too)

The NTFS timestomp test file had this as its only test:
self.assertEqual(True, True)
The domain test file was 22 lines and only checked that the class could be
instantiated. The feature extraction test file had a TODO comment and nothing
else. And DataFinder was silently accepting any string as a date without
ever checking if it was actually ISO 8601.

What this PR does

ntfs_timestomp_test.py — 5 new behavioral tests for is_suspicious():

No std_info_event → returns False
Empty file_names → returns False
Timestamps within threshold → not flagged, no attributes set
Timestamps above threshold → flagged, time_delta / time_deltas set correctly
Mixed file names (one within threshold) → detection correctly suppressed
domain_test.py — 4 new MockDataStore-backed behavioral tests:
Empty event stream → graceful early exit with "No domains to analyze."
Events with url field (no domain) → correct domain extraction
Rare domain threshold → domain counted in result summary
Known CDN domain (.cloudfront.net) → reported in CDN network count
feature_extraction_test.py — 3 new tests for FeatureExtractionSketchPlugin:
Getters and setters work for plugin_name, feature_name, feature_config
Empty plugin_name → returns "Feature extraction plugin name is empty" cleanly
Unregistered plugin name → gracefully returns wrapped ValueError message
datafinder.py — ISO 8601 date validation:
Added _is_valid_iso_date() helper using datetime.fromisoformat()
set_start_date() and set_end_date() now log a warning if the input isn't valid
Non-breaking: the value is still set regardless, so no existing callers are affected
Removes both TODO comments

Checks

All tests pass
Unit tests added
e2e tests added — not applicable; changes are pure unit-level with no new endpoints or UI behavior
Documentation updated — not applicable; no public API or user-facing behavior changed
Closing issues
Closes Resolve inline TODOs: missing analyzer tests and datafinder date validation #3782

jaegeral · 2026-03-26T06:50:01Z

/gemini review

SNO7E-G · 2026-03-26T14:41:51Z

@jaegeral
Done! I've fixed all 3 CI failures and pushed the corrected commit

jkppr

Thanks for the contribution. I have some comments below.

jkppr · 2026-06-10T15:28:43Z

    @mock.patch("timesketch.lib.analyzers.interface.OpenSearchDataStore", MockDataStore)
-    def test_analyzer(self):
-        """Test analyzer."""
-        # TODO: Write actual tests here.


The original TODO was sitting under a placeholder test_analyzer(self). The goal of that TODO was to test the analyzer's main run() method (which queries the datastore, groups events, and creates the Timesketch view), rather than just the is_suspicious() helper.

Currently, the run() method is completely untested. Would you be open to helping us write an integration test for it? It would look something like this:

Populating the MockDataStore with a couple of mock events (one with attribute_type: 16 and one with 48).

Running analyzer.run().

Verifying that the events are updated/committed and the NtfsTimestomp view is added.

jkppr · 2026-06-10T15:31:05Z

+        self.assertFalse(analyzer.is_suspicious(file_info))
+
+    @mock.patch("timesketch.lib.analyzers.interface.OpenSearchDataStore", MockDataStore)
+    def test_is_suspicious_within_threshold(self):


Isn't this already covered by the existing test_is_suspicious test? (Case 1, 4, and 5 - The existing tests already thoroughly prove that any analyzer.threshold (which includes analyzer.threshold - 1) is correctly ignored.)

jkppr · 2026-06-10T15:31:45Z

+        self.assertIsNone(fn_event.source.get("time_delta"))
+
+    @mock.patch("timesketch.lib.analyzers.interface.OpenSearchDataStore", MockDataStore)
+    def test_is_suspicious_timestomped(self):


Same here, this is also already covered by the test_is_suspicious if I'm not mistaken. (case 2 - It tests the exact same success path and attribute-setting behavior, just with different numbers.)

jkppr · 2026-06-10T15:32:46Z

+        self.assertEqual(si_event.source.get("time_deltas"), [large_diff])
+
+    @mock.patch("timesketch.lib.analyzers.interface.OpenSearchDataStore", MockDataStore)
+    def test_is_suspicious_mixed_file_names(self):


Same here, this seems also to be already covered by test_is_suspicious. (case 3 & 4 - This is the exact same "mixed list with early exit" scenario that the new test is trying to cover.)

jkppr · 2026-06-10T15:39:09Z

+        }
+
+        result = analyzer.run()
+        self.assertIn("2 domains discovered", result)


Great job setting up MockDataStore and running analyzer.run().

Currently, the assertions only check the high-level summary string returned by run() (e.g., "2 domains discovered"). To ensure that the analyzer is actually modifying the events in the datastore as expected (and to prevent future code changes from silently breaking this), we should assert the actual event tags and attributes too.

jkppr · 2026-06-10T15:45:56Z

 logger = logging.getLogger("timesketch.data_finder")


+def _is_valid_iso_date(date_string):


That should work well. Would you mind adding a timesketch/lib/datafinder_test.py file with some unit tests that verify set_start_date() and set_and_date() accept valid ISO dates and correctly log warnings for invalid dates?

SNO7E-G added 2 commits March 25, 2026 23:03

test: add missing analyzer unit tests and datafinder date validation

47e156c

Merge branch 'master' into test/analyzer-coverage

0a1fcce

jaegeral and others added 2 commits March 26, 2026 09:35

Merge branch 'master' into test/analyzer-coverage

28bc9f8

Merge branch 'master' into test/analyzer-coverage

4c92dfd

SNO7E-G force-pushed the test/analyzer-coverage branch from 27321bc to 4c92dfd Compare March 26, 2026 14:36

jkppr assigned SNO7E-G Jun 10, 2026

jkppr self-requested a review June 10, 2026 15:15

Merge branch 'master' into test/analyzer-coverage

56bdcb0

jkppr requested changes Jun 10, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

test: add missing analyzer unit tests and datafinder date validation#3783

test: add missing analyzer unit tests and datafinder date validation#3783
SNO7E-G wants to merge 5 commits into
google:masterfrom
SNO7E-G:test/analyzer-coverage

SNO7E-G commented Mar 25, 2026 •

edited

Loading

Uh oh!

jaegeral commented Mar 26, 2026

Uh oh!

SNO7E-G commented Mar 26, 2026 •

edited

Loading

Uh oh!

jkppr left a comment

Uh oh!

jkppr Jun 10, 2026

Uh oh!

jkppr Jun 10, 2026

Uh oh!

jkppr Jun 10, 2026

Uh oh!

jkppr Jun 10, 2026

Uh oh!

jkppr Jun 10, 2026

Uh oh!

jkppr Jun 10, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

		logger = logging.getLogger("timesketch.data_finder")


		def _is_valid_iso_date(date_string):

Uh oh!

Conversation

SNO7E-G commented Mar 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Hi Team 👋 This PR cleans up four TODO comments that have been sitting in the codebase for a while — all of them asking for test coverage or input validation that was never implemented. Nothing drastic, just filling in the gaps. Closes #3782

What's been bothering me (and hopefully you too)

What this PR does

Checks

Uh oh!

jaegeral commented Mar 26, 2026

Uh oh!

SNO7E-G commented Mar 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jkppr left a comment

Choose a reason for hiding this comment

Uh oh!

jkppr Jun 10, 2026

Choose a reason for hiding this comment

Uh oh!

jkppr Jun 10, 2026

Choose a reason for hiding this comment

Uh oh!

jkppr Jun 10, 2026

Choose a reason for hiding this comment

Uh oh!

jkppr Jun 10, 2026

Choose a reason for hiding this comment

Uh oh!

jkppr Jun 10, 2026

Choose a reason for hiding this comment

Uh oh!

jkppr Jun 10, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

SNO7E-G commented Mar 25, 2026 •

edited

Loading

Hi Team 👋
This PR cleans up four `TODO` comments that have been sitting in the codebase for
a while — all of them asking for test coverage or input validation that was never
implemented. Nothing drastic, just filling in the gaps.
Closes #3782

SNO7E-G commented Mar 26, 2026 •

edited

Loading