refactor(tests): `assert.Error` -> `require.Error` (other `workflow/` dirs) #13402

Joibel · 2024-07-26T17:47:25Z

This is part of a series of test tidies started by #13365.

The aim is to enable the testifylint golangci-lint checker.

This commit converts assert.Error checks into require.Error for the part of the codebase, as per #13270 (comment)

In some places checks have been coalesced - in particular the pattern

if assert.Error() {
    assert.Contains(..., "message")
}

is now

require.ErrorContains(..., "message")

Getting this wrong and missing the Contains is still valid Go, so that's a mistake I may have made.

Signed-off-by: Alan Clucas alan@clucas.org

terrytangyuan

Can we combine these PRs into one?

Joibel · 2024-07-27T18:25:55Z

Can we combine these PRs into one?

I've split these to make them easier to digest - it's otherwise a heavy but dull change to over 100 files. It's @agilgur5 and my preference to work on smaller commits. These are arbitary splits, each file could be independent or it could be a mega commit.

I can pop them into one mega commit if they're not through when I get back from vacation.

agilgur5 · 2024-07-27T18:39:19Z

it's otherwise a heavy but dull change to over 100 files

I think closer to ~150, it's quite a lot of files. Most of the changes are straight replace, but not quite all of them (in particular the if assert.NoError ones (the if is no longer necessary when using require). So some files need a more careful double-check

This is part of a series of test tidies started by #13365. The aim is to enable the testifylint golangci-lint checker. This commit converts assert.Error checks into require.Error for the part of the codebase, as per #13270 (comment) In some places checks have been coaleced - in particular the pattern ```go if assert.Error() { assert.Contains(..., "message") } ``` is now ```go require.ErrorContains(..., "message") ``` Getting this wrong and missing the Contains is still valid go, so that's a mistake I may have made. Signed-off-by: Alan Clucas <alan@clucas.org>

agilgur5 · 2024-07-28T17:25:52Z

it's otherwise a heavy but dull change to over 100 files

I think closer to ~150

Correction, after fixing what seems to be a GH rebase bug, this did end up smaller than that, ~99 on the dot I think

agilgur5

It looks like there could be some potential further simplifications in a few places

agilgur5 · 2024-07-29T16:07:12Z

workflow/templateresolution/context_test.go

-	if !assert.NoError(t, err) {
-		t.Fatal(err)
-	}
+	require.NoError(t, err)


there's some inconsistent new lines in the changes here, so not sure which one you were going for; adding a new line to all or no new line for all

agilgur5 · 2024-07-29T16:08:14Z

workflow/templateresolution/context_test.go

 	if !assert.True(t, ok) {
 		t.Fatal("tmplBase is not a WorkflowTemplate")


can this also be simplified?

workflow/artifacts/http/clients_test.go

agilgur5 · 2024-07-29T16:16:31Z

workflow/util/util_test.go

+		if assert.Len(t, wf.Status.Nodes, 2) {
+			assert.Equal(t, wfv1.NodeSucceeded, wf.Status.Nodes["1"].Phase)
+			assert.Equal(t, wfv1.NodeRunning, wf.Status.Nodes["continue-on-failed-workflow-2"].Phase)
+			assert.Len(t, podsToDelete, 4)
 		}


Suggested change

if assert.Len(t, wf.Status.Nodes, 2) {

assert.Equal(t, wfv1.NodeSucceeded, wf.Status.Nodes["1"].Phase)

assert.Equal(t, wfv1.NodeRunning, wf.Status.Nodes["continue-on-failed-workflow-2"].Phase)

assert.Len(t, podsToDelete, 4)

}

require.Len(t, wf.Status.Nodes, 2)

assert.Equal(t, wfv1.NodeSucceeded, wf.Status.Nodes["1"].Phase)

assert.Equal(t, wfv1.NodeRunning, wf.Status.Nodes["continue-on-failed-workflow-2"].Phase)

assert.Len(t, podsToDelete, 4)

this can also be simplified as a require if I'm not mistaken

see also my comment on the other PR: #13401 (comment)

these are not assert.Error changes though, so may make sense as a separate PR. as it would be another indentation change though, it may make sense to consolidate for diff simplicity

agilgur5 · 2024-07-29T16:17:44Z

workflow/util/util_test.go

@@ -602,7 +599,7 @@ func TestApplySubmitOpts(t *testing.T) {
 			},
 		}
 		err := ApplySubmitOpts(wf, &wfv1.SubmitOpts{Parameters: []string{"a=81861780812"}})
-		assert.NoError(t, err)
+		require.NoError(t, err)
 		parameters := wf.Spec.Arguments.Parameters
 		if assert.Len(t, parameters, 1) {


same as above

agilgur5 · 2024-07-29T16:19:49Z

workflow/common/convert_test.go

@@ -106,16 +107,16 @@ spec:
 `

 	err = yaml.Unmarshal([]byte(cronWfInstanceIdString), &cronWf)
-	assert.NoError(t, err)
+	require.NoError(t, err)
 	wf = ConvertCronWorkflowToWorkflow(&cronWf)
 	if assert.Contains(t, wf.GetLabels(), LabelKeyControllerInstanceID) {


same as above

agilgur5 · 2024-07-29T16:21:09Z

workflow/artifacts/http/http_test.go

-			}
+		require.Error(t, err)
+		argoError, ok := err.(errors.ArgoError)
+		if assert.True(t, ok) {


same as above.

I also wonder if this is further simplifiable with the Error* helpers

agilgur5 · 2024-07-29T16:21:15Z

workflow/artifacts/http/http_test.go

-			}
+		require.Error(t, err)
+		argoError, ok := err.(errors.ArgoError)
+		if assert.True(t, ok) {


same as above

agilgur5 · 2024-07-29T16:22:17Z

workflow/artifacts/common/load_to_stream_test.go

@@ -115,7 +116,7 @@ func TestLoadToStream(t *testing.T) {
 				}


can the panic up here also be replaced by a require.NoError? there's a couple panics in the tests that seem like they should perhaps be a require.NoError from a quick glance

agilgur5 · 2024-07-29T16:24:28Z

workflow/artifacts/azure/azure_test.go

@@ -62,11 +63,11 @@ func TestArtifactDriver_WithServiceKey_DownloadDirectory_Subdir(t *testing.T) {

 	// ensure container exists
 	containerClient, err := driver.newAzureContainerClient()
-	assert.NoError(t, err)
+	require.NoError(t, err)
 	_, err = containerClient.Create(context.Background(), nil)
 	var responseError *azcore.ResponseError
 	if err != nil && !(errors.As(err, &responseError) && responseError.ErrorCode == "ContainerAlreadyExists") {


wondering if this might be able to be rewritten as assertions. maybe not though

Joibel · 2024-07-30T06:12:27Z

Feel free to apply changes as you see fit @agilgur5

agilgur5

LGTM. leaving the non-error if assert.* -> require.* changes to a separate PR per #13402 (comment). same for the panic -> require etc changes

terrytangyuan reviewed Jul 27, 2024

View reviewed changes

Base automatically changed from testifylint-automated-fix to main July 27, 2024 16:05

agilgur5 changed the title ~~chore(tests): assert.Error->require.Error (workflow/other directory)~~ refactor(tests): assert.Error -> require.Error (other workflow/ dirs) Jul 27, 2024

Joibel force-pushed the testifylint-require-workflow-other branch from 173e074 to f5a1a53 Compare July 27, 2024 18:20

Joibel force-pushed the testifylint-require-workflow-other branch from f5a1a53 to 32f195f Compare July 27, 2024 19:46

agilgur5 reviewed Jul 29, 2024

View reviewed changes

agilgur5 mentioned this pull request Jul 30, 2024

refactor(tests): assert.Error -> require.Error (workflow/controller/) #13401

Merged

agilgur5 approved these changes Aug 10, 2024

View reviewed changes

agilgur5 merged commit 24de3c6 into main Aug 10, 2024
28 checks passed

agilgur5 deleted the testifylint-require-workflow-other branch August 10, 2024 20:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(tests): `assert.Error` -> `require.Error` (other `workflow/` dirs) #13402

refactor(tests): `assert.Error` -> `require.Error` (other `workflow/` dirs) #13402

Joibel commented Jul 26, 2024 •

edited by agilgur5

Loading

terrytangyuan left a comment

Joibel commented Jul 27, 2024

agilgur5 commented Jul 27, 2024

agilgur5 commented Jul 28, 2024

agilgur5 left a comment

agilgur5 Jul 29, 2024

agilgur5 Jul 29, 2024

agilgur5 Jul 29, 2024

agilgur5 Jul 30, 2024

agilgur5 Jul 29, 2024 •

edited

Loading

agilgur5 Jul 29, 2024 •

edited

Loading

agilgur5 Jul 29, 2024 •

edited

Loading

agilgur5 Jul 29, 2024 •

edited

Loading

agilgur5 Jul 29, 2024

agilgur5 Jul 29, 2024

Joibel commented Jul 30, 2024

agilgur5 left a comment •

edited

Loading

		if !assert.True(t, ok) {
		t.Fatal("tmplBase is not a WorkflowTemplate")

refactor(tests): assert.Error -> require.Error (other workflow/ dirs) #13402

refactor(tests): assert.Error -> require.Error (other workflow/ dirs) #13402

Conversation

Joibel commented Jul 26, 2024 • edited by agilgur5 Loading

terrytangyuan left a comment

Choose a reason for hiding this comment

Joibel commented Jul 27, 2024

agilgur5 commented Jul 27, 2024

agilgur5 commented Jul 28, 2024

agilgur5 left a comment

Choose a reason for hiding this comment

agilgur5 Jul 29, 2024

Choose a reason for hiding this comment

agilgur5 Jul 29, 2024

Choose a reason for hiding this comment

agilgur5 Jul 29, 2024

Choose a reason for hiding this comment

agilgur5 Jul 30, 2024

Choose a reason for hiding this comment

agilgur5 Jul 29, 2024 • edited Loading

Choose a reason for hiding this comment

agilgur5 Jul 29, 2024 • edited Loading

Choose a reason for hiding this comment

agilgur5 Jul 29, 2024 • edited Loading

Choose a reason for hiding this comment

agilgur5 Jul 29, 2024 • edited Loading

Choose a reason for hiding this comment

agilgur5 Jul 29, 2024

Choose a reason for hiding this comment

agilgur5 Jul 29, 2024

Choose a reason for hiding this comment

Joibel commented Jul 30, 2024

agilgur5 left a comment • edited Loading

Choose a reason for hiding this comment

refactor(tests): `assert.Error` -> `require.Error` (other `workflow/` dirs) #13402

refactor(tests): `assert.Error` -> `require.Error` (other `workflow/` dirs) #13402

Joibel commented Jul 26, 2024 •

edited by agilgur5

Loading

agilgur5 Jul 29, 2024 •

edited

Loading

agilgur5 Jul 29, 2024 •

edited

Loading

agilgur5 Jul 29, 2024 •

edited

Loading

agilgur5 Jul 29, 2024 •

edited

Loading

agilgur5 left a comment •

edited

Loading