Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

chore: cleanup errchecking in tests #3040

Merged
merged 37 commits into from
Sep 30, 2024
Merged

chore: cleanup errchecking in tests #3040

merged 37 commits into from
Sep 30, 2024

Conversation

mkcp
Copy link
Contributor

@mkcp mkcp commented Sep 26, 2024 •
edited
Loading

Description

This PR is split out from #2993 to only fix ignored errors in tests.

Related Issue

Fixes #2949
Relates to #2993, #2953

Checklist before merging

@mkcp mkcp self-assigned this Sep 26, 2024
@mkcp mkcp requested review from a team as code owners September 26, 2024 17:08
@netlify Netlify
Copy link

netlify bot commented Sep 26, 2024 •
edited
Loading

✅ Deploy Preview for zarf-docs canceled.

Name Link
🔨 Latest commit e23dd79
🔍 Latest deploy log https://app.netlify.com/sites/zarf-docs/deploys/66faf0ad19707100088cfceb

mkcp added 26 commits September 26, 2024 10:26
@mkcp
add errcheck linting
8c16eda 
Signed-off-by: Kit Patella <kit@defenseunicorns.com>
@mkcp
linter ignore some intentionally unhandled errs
430467e 
Signed-off-by: Kit Patella <kit@defenseunicorns.com>
@mkcp
handle some env var set and unset errors in tests
2e093b1 
Signed-off-by: Kit Patella <kit@defenseunicorns.com>
@mkcp
make some notes in cmd/destroy.go for later
0e9405e 
Signed-off-by: Kit Patella <kit@defenseunicorns.com>
@mkcp
direct handle some deferred cleanups to add direct error handling
5ad8530 
Signed-off-by: Kit Patella <kit@defenseunicorns.com>
@mkcp
capture errors from cmd/root
23663e4 
Signed-off-by: Kit Patella <kit@defenseunicorns.com>
@mkcp
capture errors on src/config/config.go. checking ci
7645357 
Signed-off-by: Kit Patella <kit@defenseunicorns.com>
@mkcp
fix: ensure we capture and propagate errors in src/internal
4b5c794 
Signed-off-by: Kit Patella <kit@defenseunicorns.com>
@mkcp
fix: propagate or log unhandled errors in src/cmd
849680d 
Signed-off-by: Kit Patella <kit@defenseunicorns.com>
@mkcp
fix: check for or intentionally ignore some dangling errors in tests
433aacd 
Signed-off-by: Kit Patella <kit@defenseunicorns.com>
@mkcp
fix: use RemoveAll not remove so it doesnt error every time
b579634 
Signed-off-by: Kit Patella <kit@defenseunicorns.com>
@mkcp
fix: disabling the err catch to see if it's something i changed
7b418d6 
Signed-off-by: Kit Patella <kit@defenseunicorns.com>
@mkcp
fix: should fix the directory not empty panic
e8e4ee9 
Signed-off-by: Kit Patella <kit@defenseunicorns.com>
@mkcp
fix: see if CI is flaking with the original tests. probably just need…
73cb6a4 
… to nolint ignore them

Signed-off-by: Kit Patella <kit@defenseunicorns.com>
@mkcp
fix: these tests should clean up properly and not error
81b8c05 
Signed-off-by: Kit Patella <kit@defenseunicorns.com>
@mkcp
fix: actually cleaning up the test with RemovalAll seems to cause a p…
cf43754 
…anic

Signed-off-by: Kit Patella <kit@defenseunicorns.com>
@mkcp
fix: reintroduce the ext_in_cluster test error catches
2f1bd63 
Signed-off-by: Kit Patella <kit@defenseunicorns.com>
@mkcp
fix: missed a spot
f377bb0 
Signed-off-by: Kit Patella <kit@defenseunicorns.com>
@mkcp
fix: e2e 24 - can we assert that this error exists or do we have to i…
4d87c74 
…gnore it

Signed-off-by: Kit Patella <kit@defenseunicorns.com>
@mkcp
fix: address some fixmes before merge
1676109 
Signed-off-by: Kit Patella <kit@defenseunicorns.com>
@mkcp
fix: does this work now?
8e36039 
Signed-off-by: Kit Patella <kit@defenseunicorns.com>
@mkcp
fix: handle and propagate errors in pkg/utils
76a6665 
Signed-off-by: Kit Patella <kit@defenseunicorns.com>
@mkcp
fix: propagate any errors in ColorPrintYAML
c6e592d 
Signed-off-by: Kit Patella <kit@defenseunicorns.com>
@mkcp
fix: propagate any errors in src/pkg/message
752f0ff 
Signed-off-by: Kit Patella <kit@defenseunicorns.com>
@mkcp
fix: propagate any errors in src/pkg/variables
cbb192e 
Signed-off-by: Kit Patella <kit@defenseunicorns.com>
@mkcp
fix: propagate any errors in src/pkg/layout
9eb3ef0 
Signed-off-by: Kit Patella <kit@defenseunicorns.com>
mkcp added 6 commits September 26, 2024 10:26
@mkcp
fix: srcfile is already closed and the error handled, no need to defe…
0e6ef61 
…r before loop

Signed-off-by: Kit Patella <kit@defenseunicorns.com>
@mkcp
fix: ensure errors from deferred resource cleanup are passed back to …
7879eb8 
…caller

Signed-off-by: Kit Patella <kit@defenseunicorns.com>
@mkcp
chore: undo linter rule for CI so we can merge incremental changes
1c471d2 
Signed-off-by: Kit Patella <kit@defenseunicorns.com>
@mkcp
chore: undo the other linter rule
a62fba9 
Signed-off-by: Kit Patella <kit@defenseunicorns.com>
@mkcp
chore: undo non-testing changes
3ddd3dc 
Signed-off-by: Kit Patella <kit@defenseunicorns.com>
@mkcp
fix: remove nolints for now to make ci happy. we'll have to readd later
20241aa 
Signed-off-by: Kit Patella <kit@defenseunicorns.com>
@mkcp mkcp force-pushed the 2949-test-errchecking branch from a8e5de9 to 20241aa Compare September 26, 2024 17:26
@codecov Codecov
Copy link

codecov bot commented Sep 26, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

AustinAbro321
AustinAbro321 requested changes Sep 26, 2024
View reviewed changes
Copy link
Contributor

@AustinAbro321 AustinAbro321 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only one comment, but it applies to most of the defers that got deleted. I think we should keep the defers, and check errors within them otherwise during when a test fails before the cleanup step there will be no cleanup.

src/test/e2e/14_oci_compose_test.go Outdated Show resolved Hide resolved
mkcp added 2 commits September 26, 2024 12:10
@mkcp
fix: ensure we're defering resource cleanup in the tests
9d5eced 
Signed-off-by: Kit Patella <kit@defenseunicorns.com>
@mkcp
chore: mark todos where we'll need to nolint later
93417ee 
Signed-off-by: Kit Patella <kit@defenseunicorns.com>
@mkcp
Copy link
Contributor Author

mkcp commented Sep 26, 2024

@AustinAbro321 Defers should be fixed in latest, also added some todos for when we swap over to the linter in main

schristoff
schristoff previously approved these changes Sep 27, 2024
View reviewed changes
src/test/e2e/29_config_file_test.go Outdated
@@ -139,30 +143,37 @@ func configFileDefaultTests(t *testing.T) {
}

// Test remaining default initializers
t.Setenv("ZARF_CONFIG", filepath.Join("src", "test", "zarf-config-test.toml"))
defer os.Unsetenv("ZARF_CONFIG")
envKey := "ZARF_CONFIG"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Could just set as const mayb?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

def, opted to keep it consistent but const was preferred

AustinAbro321
AustinAbro321 requested changes Sep 27, 2024
View reviewed changes
Copy link
Contributor

@AustinAbro321 AustinAbro321 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few more comments, but it's very close. Nice work! I've learned a lot about defer while doing this reviews

src/test/common.go Show resolved Hide resolved
src/test/e2e/21_connect_creds_test.go Outdated Show resolved Hide resolved
src/test/e2e/14_oci_compose_test.go Show resolved Hide resolved
@mkcp
chore: ensure NoError e2e.CleanFiles and pass thru testing.T
cc6b253 
Signed-off-by: Kit Patella <kit@defenseunicorns.com>
@mkcp
Copy link
Contributor Author

mkcp commented Sep 30, 2024

A few more comments, but it's very close. Nice work! I've learned a lot about defer while doing this reviews

Should all be reserved (cc @schristoff) and yeah 100% here too. I ended up trying a bunch of different error handling patterns, from manual orders to the named err + join, and definitely added a few tricks to the toolbox. named err + join is probably the closest thing we have in Go to a Finally block.

mkcp added 2 commits September 30, 2024 11:40
@mkcp
chore: use const
838e1e0 
Signed-off-by: Kit Patella <kit@defenseunicorns.com>
@mkcp
chore: ensure we don't try to close a nil response body
e23dd79 
Signed-off-by: Kit Patella <kit@defenseunicorns.com>
@mkcp mkcp added this pull request to the merge queue Sep 30, 2024
Merged via the queue into main with commit 59b373a Sep 30, 2024
30 checks passed
@mkcp mkcp deleted the 2949-test-errchecking branch September 30, 2024 19:30
ittacco pushed a commit to ittacco/zarf that referenced this pull request Nov 9, 2024
@mkcp @ittacco
chore: cleanup errchecking in tests (zarf-dev#3040)
6e15d53 
Signed-off-by: Kit Patella <kit@defenseunicorns.com>
Signed-off-by: ittacco <lorenzo.tacconi1967@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AustinAbro321 AustinAbro321 AustinAbro321 approved these changes

@schristoff schristoff schristoff left review comments

Assignees

@mkcp mkcp

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Enable linting ignored
3 participants
@mkcp @schristoff @AustinAbro321