From a2468fa0b47963410b0d3b4fa7da61937a8241ee Mon Sep 17 00:00:00 2001
From: Ian Littman <iansltx@gmail.com>
Date: Tue, 5 Nov 2024 07:34:54 -0600
Subject: [PATCH] Re-enable TestCronVulnerabilitiesCreatesDatabasesPath test
 with more precise SUT and internal panic recovery (#23468)

#23258 (see
[comment](https://github.com/fleetdm/fleet/issues/23258#issuecomment-2443304838)
for rationale)

Validated by removing the two places that would create the directory
(early in scanVulnerabilities in cron.go, partway through download in
download.go) and ensuring the test failed (timeout after 10s).

Both dir creations happen early in the vulns cron so I was able to
drastically tighten the timing on the periodic check on this test, so
this tests completes way quicker than before as an added benefit
(automatic test parallelism notwithstanding).

The panic recovery here theoretically shouldn't be necessary, as on a
passed test the context will get cancelled while syncing the CPE sqlite,
but is included to ensure the test doesn't flake if the implementation
of the vulnerabilities cron changes such that we _would_ get a panic by
cancelling the context this early.

# Checklist for submitter
- [x] Added/updated tests
---
 cmd/fleet/serve_test.go | 33 +++++++++++++++++----------------
 1 file changed, 17 insertions(+), 16 deletions(-)

diff --git a/cmd/fleet/serve_test.go b/cmd/fleet/serve_test.go
index ff3260358c..24175ef1ba 100644
--- a/cmd/fleet/serve_test.go
+++ b/cmd/fleet/serve_test.go
@@ -307,7 +307,6 @@ func TestAutomationsSchedule(t *testing.T) {
 }
 
 func TestCronVulnerabilitiesCreatesDatabasesPath(t *testing.T) {
-	t.Skip() // https://github.com/fleetdm/fleet/issues/23258
 	t.Parallel()
 	ctx, cancelFunc := context.WithCancel(context.Background())
 	defer cancelFunc()
@@ -355,24 +354,26 @@ func TestCronVulnerabilitiesCreatesDatabasesPath(t *testing.T) {
 	ctx = license.NewContext(ctx, &fleet.LicenseInfo{Tier: fleet.TierPremium})
 	ctx, cancel := context.WithCancel(ctx)
 	lg := kitlog.NewJSONLogger(os.Stdout)
-	s, err := newVulnerabilitiesSchedule(ctx, "test_instance", ds, lg, &config)
-	require.NoError(t, err)
-	s.Start()
-	t.Cleanup(func() {
-		cancel()
-		<-s.Done()
-	})
+
+	go func() {
+		defer func() {
+			// this test may panic as we're ending it early, so we shouldn't fail our suite on this panic
+			// but depending on where we are in the cron call it may not panic, hence not checking the recover
+			// value either way
+			// nolint:errcheck
+			recover()
+		}()
+		_ = cronVulnerabilities(ctx, ds, lg, &config)
+	}()
 
 	assert.Eventually(t, func() bool {
 		info, err := os.Lstat(vulnPath)
-		if err != nil {
-			return false
-		}
-		if !info.IsDir() {
-			return false
-		}
-		return true
-	}, 5*time.Minute, 30*time.Second)
+		return err == nil && info.IsDir()
+	}, 10*time.Second, 1*time.Second)
+
+	t.Cleanup(func() {
+		cancel()
+	})
 
 	// at this point, the assertion has succeeded or failed, try to remove the
 	// temp dir and all content instead of leaving it to the testing package to