Skip to content

Commit 23654f4

Browse files
vjtvjt
committed
Cleanup steps: do not require a cleanup test name
As the cleanup steps are strictly related to the test they refer to, remove the requirement to be in a named test. Signed-off-by: Marcello Barnaba <mbarnaba@meta.com>
1 parent 0f7af7c commit 23654f4

12 files changed

Lines changed: 30 additions & 49 deletions

File tree

‎cmds/clients/contestcli/descriptors/start-literal-with-cleanup.json‎

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -38,7 +38,6 @@
3838
},
3939
"CleanupFetcherName": "literal",
4040
"CleanupFetcherFetchParameters": {
41-
"TestName": "cleanup",
4241
"Steps": [
4342
{
4443
"name": "cmd",
@@ -67,4 +66,3 @@
6766
]
6867
}
6968
}
70-

‎pkg/jobmanager/job.go‎

Lines changed: 1 addition & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -122,19 +122,12 @@ func buildTestsFromDescriptors(
122122
return nil, err
123123
}
124124

125-
var (
126-
bundleCleanup []test.TestStepBundle
127-
cleanupName string
128-
)
125+
var bundleCleanup []test.TestStepBundle
129126
if len(thisTestStepsDescriptors.CleanupSteps) > 0 {
130127
bundleCleanup, err = newBundlesFromSteps(ctx, thisTestStepsDescriptors.CleanupSteps, registry)
131128
if err != nil {
132129
return nil, fmt.Errorf("could not create cleanup test steps bundles: %w", err)
133130
}
134-
cleanupName = thisTestStepsDescriptors.CleanupName
135-
if err := limits.NewValidator().ValidateTestName(cleanupName); err != nil {
136-
return nil, err
137-
}
138131
}
139132
if err := validateNoDuplicateLabels(append(bundleTest, bundleCleanup...)); err != nil {
140133
return nil, err
@@ -148,7 +141,6 @@ func buildTestsFromDescriptors(
148141
TestFetcherBundle: bundleTestFetcher,
149142
TestStepsBundles: bundleTest,
150143
RetryParameters: td.RetryParameters,
151-
CleanupName: cleanupName,
152144
CleanupStepsBundles: bundleCleanup,
153145
}
154146
tests = append(tests, &test)

‎pkg/jobmanager/steps.go‎

Lines changed: 2 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -44,12 +44,9 @@ func (f fetcherStepsResolver) GetStepsDescriptors(ctx xcontext.Context) ([]test.
4444
return nil, err
4545
}
4646

47-
var (
48-
cleanupName string
49-
cleanupDescriptors []*test.TestStepDescriptor
50-
)
47+
var cleanupDescriptors []*test.TestStepDescriptor
5148
if bundleTestFetcher.CleanupFetcher != nil {
52-
cleanupName, cleanupDescriptors, err = bundleTestFetcher.CleanupFetcher.Fetch(ctx, bundleTestFetcher.CleanupParameters)
49+
_, cleanupDescriptors, err = bundleTestFetcher.CleanupFetcher.Fetch(ctx, bundleTestFetcher.CleanupParameters)
5350
if err != nil {
5451
return nil, err
5552
}
@@ -60,7 +57,6 @@ func (f fetcherStepsResolver) GetStepsDescriptors(ctx xcontext.Context) ([]test.
6057
test.TestStepsDescriptors{
6158
TestName: testName,
6259
TestSteps: stepDescriptors,
63-
CleanupName: cleanupName,
6460
CleanupSteps: cleanupDescriptors,
6561
},
6662
)

‎pkg/pluginregistry/bundles.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -56,7 +56,7 @@ func (r *PluginRegistry) NewTestFetcherBundle(ctx xcontext.Context, testDescript
5656
return nil, fmt.Errorf("could not get the desired TestFetcher (%s): %v", testDescriptor.TestFetcherName, err)
5757
}
5858
// FetchParameters
59-
fetchParameters, err := testFetcher.ValidateFetchParameters(ctx, testDescriptor.TestFetcherFetchParameters)
59+
fetchParameters, err := testFetcher.ValidateFetchParameters(ctx, testDescriptor.TestFetcherFetchParameters, true)
6060
if err != nil {
6161
return nil, fmt.Errorf("could not validate TestFetcher fetch parameters: %v", err)
6262
}
@@ -73,7 +73,7 @@ func (r *PluginRegistry) NewTestFetcherBundle(ctx xcontext.Context, testDescript
7373

7474
}
7575
// FetchParameters
76-
cleanupParameters, err = testFetcher.ValidateFetchParameters(ctx, testDescriptor.CleanupFetcherFetchParameters)
76+
cleanupParameters, err = testFetcher.ValidateFetchParameters(ctx, testDescriptor.CleanupFetcherFetchParameters, false)
7777
if err != nil {
7878
return nil, fmt.Errorf("could not validate CleanupFetcher fetch parameters: %v", err)
7979
}

‎pkg/runner/job_runner.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -491,7 +491,7 @@ func (jr *JobRunner) runTest(ctx xcontext.Context,
491491
runCtx,
492492
t,
493493
targets,
494-
NewTestStepEventsEmitterFactory(jr.storageEngineVault, j.ID, runID, t.CleanupName, testAttempt),
494+
NewTestStepEventsEmitterFactory(jr.storageEngineVault, j.ID, runID, fmt.Sprintf("%s [CLEANUP]", t.Name), testAttempt),
495495
cleanupRunnerState,
496496
stepOutputs,
497497
t.CleanupStepsBundles,

‎pkg/runner/job_runner_test.go‎

Lines changed: 16 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -156,8 +156,7 @@ func (s *JobRunnerSuite) TestJobWithCleanupStepsStartToFinish() {
156156
TargetManagerReleaseTimeout: 10 * time.Second,
157157
Tests: []*test.Test{
158158
{
159-
Name: testName,
160-
CleanupName: testName,
159+
Name: testName,
161160
TargetManagerBundle: &target.TargetManagerBundle{
162161
AcquireParameters: acquireParameters,
163162
TargetManager: targetlist.New(),
@@ -186,10 +185,10 @@ func (s *JobRunnerSuite) TestJobWithCleanupStepsStartToFinish() {
186185
{[1 1 SimpleTest 0 ][Target{ID: "T1"} TargetAcquired]}
187186
{[1 1 SimpleTest 0 test_step_label][Target{ID: "T1"} TargetIn]}
188187
{[1 1 SimpleTest 0 test_step_label][Target{ID: "T1"} TargetOut]}
189-
{[1 1 SimpleTest 0 test_cleanup_step][Target{ID: "T1"} TargetIn]}
190-
{[1 1 SimpleTest 0 test_cleanup_step][Target{ID: "T1"} TargetOut]}
188+
{[1 1 SimpleTest [CLEANUP] 0 test_cleanup_step][Target{ID: "T1"} TargetIn]}
189+
{[1 1 SimpleTest [CLEANUP] 0 test_cleanup_step][Target{ID: "T1"} TargetOut]}
191190
{[1 1 SimpleTest 0 ][Target{ID: "T1"} TargetReleased]}
192-
`, s.MemoryStorage.GetTargetEvents(ctx, []string{testName}, "T1"))
191+
`, s.MemoryStorage.GetTargetEvents(ctx, []string{testName, fmt.Sprintf("%s [CLEANUP]", testName)}, "T1"))
193192
}
194193

195194
func (s *JobRunnerSuite) TestJobWithTestRetry() {
@@ -243,8 +242,7 @@ func (s *JobRunnerSuite) TestJobWithTestRetry() {
243242
},
244243
Tests: []*test.Test{
245244
{
246-
Name: testName,
247-
CleanupName: fmt.Sprintf("%s:CLEANUP", testName),
245+
Name: testName,
248246
RetryParameters: test.RetryParameters{
249247
NumRetries: 1,
250248
RetryInterval: xjson.Duration(time.Millisecond), // make a small interval to test waiting branch
@@ -283,8 +281,8 @@ func (s *JobRunnerSuite) TestJobWithTestRetry() {
283281
{[1 1 SimpleTest 0 echo1_step_label][Target{ID: "T1"} TargetOut]}
284282
{[1 1 SimpleTest 0 test_step_label][Target{ID: "T1"} TargetIn]}
285283
{[1 1 SimpleTest 0 test_step_label][Target{ID: "T1"} TargetErr &"{\"Error\":\"some error\"}"]}
286-
{[1 1 SimpleTest:CLEANUP 0 test_cleanup_step][Target{ID: "T1"} TargetIn]}
287-
{[1 1 SimpleTest:CLEANUP 0 test_cleanup_step][Target{ID: "T1"} TargetOut]}
284+
{[1 1 SimpleTest [CLEANUP] 0 test_cleanup_step][Target{ID: "T1"} TargetIn]}
285+
{[1 1 SimpleTest [CLEANUP] 0 test_cleanup_step][Target{ID: "T1"} TargetOut]}
288286
{[1 1 SimpleTest 0 ][Target{ID: "T1"} TargetReleased]}
289287
{[1 1 SimpleTest 1 ][Target{ID: "T1"} TargetAcquired]}
290288
{[1 1 SimpleTest 1 echo1_step_label][Target{ID: "T1"} TargetIn]}
@@ -293,10 +291,10 @@ func (s *JobRunnerSuite) TestJobWithTestRetry() {
293291
{[1 1 SimpleTest 1 test_step_label][Target{ID: "T1"} TargetOut]}
294292
{[1 1 SimpleTest 1 echo2_step_label][Target{ID: "T1"} TargetIn]}
295293
{[1 1 SimpleTest 1 echo2_step_label][Target{ID: "T1"} TargetOut]}
296-
{[1 1 SimpleTest:CLEANUP 1 test_cleanup_step][Target{ID: "T1"} TargetIn]}
297-
{[1 1 SimpleTest:CLEANUP 1 test_cleanup_step][Target{ID: "T1"} TargetOut]}
294+
{[1 1 SimpleTest [CLEANUP] 1 test_cleanup_step][Target{ID: "T1"} TargetIn]}
295+
{[1 1 SimpleTest [CLEANUP] 1 test_cleanup_step][Target{ID: "T1"} TargetOut]}
298296
{[1 1 SimpleTest 1 ][Target{ID: "T1"} TargetReleased]}
299-
`, s.MemoryStorage.GetTargetEvents(ctx, []string{testName, fmt.Sprintf("%s:CLEANUP", testName)}, "T1"))
297+
`, s.MemoryStorage.GetTargetEvents(ctx, []string{testName, fmt.Sprintf("%s [CLEANUP]", testName)}, "T1"))
300298

301299
require.Len(s.T(), reporter.runStatuses, 1)
302300
require.Len(s.T(), reporter.runStatuses[0].TestStatuses, 1)
@@ -357,8 +355,7 @@ func (s *JobRunnerSuite) TestJobRetryOnFailedAcquire() {
357355
},
358356
Tests: []*test.Test{
359357
{
360-
Name: testName,
361-
CleanupName: testName,
358+
Name: testName,
362359
RetryParameters: test.RetryParameters{
363360
NumRetries: 1,
364361
RetryInterval: xjson.Duration(time.Millisecond), // make a small interval to test waiting branch
@@ -396,12 +393,12 @@ func (s *JobRunnerSuite) TestJobRetryOnFailedAcquire() {
396393
{[1 1 SimpleTest 1 ][Target{ID: "T1"} TargetAcquired]}
397394
{[1 1 SimpleTest 1 echo1_step_label][Target{ID: "T1"} TargetIn]}
398395
{[1 1 SimpleTest 1 echo1_step_label][Target{ID: "T1"} TargetOut]}
399-
{[1 1 SimpleTest 1 test_cleanup_step_1][Target{ID: "T1"} TargetIn]}
400-
{[1 1 SimpleTest 1 test_cleanup_step_1][Target{ID: "T1"} TargetOut]}
401-
{[1 1 SimpleTest 1 test_cleanup_step_2][Target{ID: "T1"} TargetIn]}
402-
{[1 1 SimpleTest 1 test_cleanup_step_2][Target{ID: "T1"} TargetOut]}
396+
{[1 1 SimpleTest [CLEANUP] 1 test_cleanup_step_1][Target{ID: "T1"} TargetIn]}
397+
{[1 1 SimpleTest [CLEANUP] 1 test_cleanup_step_1][Target{ID: "T1"} TargetOut]}
398+
{[1 1 SimpleTest [CLEANUP] 1 test_cleanup_step_2][Target{ID: "T1"} TargetIn]}
399+
{[1 1 SimpleTest [CLEANUP] 1 test_cleanup_step_2][Target{ID: "T1"} TargetOut]}
403400
{[1 1 SimpleTest 1 ][Target{ID: "T1"} TargetReleased]}
404-
`, s.MemoryStorage.GetTestEvents(ctx, []string{testName}))
401+
`, s.MemoryStorage.GetTestEvents(ctx, []string{testName, fmt.Sprintf("%s [CLEANUP]", testName)}))
405402

406403
require.Len(s.T(), reporter.runStatuses, 1)
407404
require.Len(s.T(), reporter.runStatuses[0].TestStatuses, 1)

‎pkg/test/fetcher.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,7 @@ type TestFetcherLoader func() (string, TestFetcherFactory)
2020
// TestFetcher is an interface used to get the test to run on the selected
2121
// hosts.
2222
type TestFetcher interface {
23-
ValidateFetchParameters(xcontext.Context, []byte) (interface{}, error)
23+
ValidateFetchParameters(xcontext.Context, []byte, bool) (interface{}, error)
2424
Fetch(xcontext.Context, interface{}) (string, []*TestStepDescriptor, error)
2525
}
2626

‎pkg/test/step.go‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -74,7 +74,6 @@ type TestStepLoader func() (string, TestStepFactory, []event.Name)
7474
type TestStepsDescriptors struct {
7575
TestName string
7676
TestSteps []*TestStepDescriptor
77-
CleanupName string
7877
CleanupSteps []*TestStepDescriptor
7978
}
8079

‎pkg/test/test.go‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,6 @@ type Test struct {
2626
TargetManagerBundle *target.TargetManagerBundle
2727
TestFetcherBundle *TestFetcherBundle
2828
RetryParameters RetryParameters
29-
CleanupName string
3029
CleanupStepsBundles []TestStepBundle
3130
}
3231

‎plugins/testfetchers/literal/literal.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -33,12 +33,12 @@ type Literal struct {
3333

3434
// ValidateFetchParameters performs sanity checks on the fields of the
3535
// parameters that will be passed to Fetch.
36-
func (tf Literal) ValidateFetchParameters(_ xcontext.Context, params []byte) (interface{}, error) {
36+
func (tf Literal) ValidateFetchParameters(_ xcontext.Context, params []byte, requireName bool) (interface{}, error) {
3737
var fp FetchParameters
3838
if err := json.Unmarshal(params, &fp); err != nil {
3939
return nil, err
4040
}
41-
if fp.TestName == "" {
41+
if requireName && fp.TestName == "" {
4242
return nil, fmt.Errorf("test name cannot be empty for fetch parameters")
4343
}
4444
return fp, nil

0 commit comments

Comments
 (0)