Skip to content

s/apparmor: use os.WriteFile in apparmor_test.go - #17008

Merged
olivercalder merged 1 commit into
canonical:masterfrom
natibek:use-WriteFile-in-apparmor-test
May 8, 2026
Merged

s/apparmor: use os.WriteFile in apparmor_test.go#17008
olivercalder merged 1 commit into
canonical:masterfrom
natibek:use-WriteFile-in-apparmor-test

Conversation

@natibek

@natibek natibek commented Apr 30, 2026

Copy link
Copy Markdown
Contributor

Replace several separate os.OpenFile, c.Check(err, IsNil), c.Check(file.Close(), IsNil) calls with a single os.WriteFile call.

Pointed out in #15624 (comment)

Tracked with: SNAPDENG-35228

@codecov

codecov Bot commented Apr 30, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 79.11%. Comparing base (8152be4) to head (858b2ef).

Additional details and impacted files
@@            Coverage Diff             @@
##           master   #17008      +/-   ##
==========================================
+ Coverage   79.06%   79.11%   +0.05%     
==========================================
  Files        1375     1364      -11     
  Lines      191085   190918     -167     
  Branches     2465     2465              
==========================================
- Hits       151075   151051      -24     
+ Misses      30917    30779     -138     
+ Partials     9093     9088       -5     
Flag Coverage Δ
unittests 79.11% <ø> (+0.05%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@github-actions

github-actions Bot commented Apr 30, 2026

Copy link
Copy Markdown

Tue May 5 20:00:15 UTC 2026
The following results are from: https://github.com/canonical/snapd/actions/runs/25396485467

Failures:

Skipped tests from snapd-testing-skip

If you wish to have any of the below tests run in your PR, in your PR description, add 'unskip:' followed by a copy-and-pasted list (without variants) of the below tests you wish to run (unskip plus test list must be valid yaml)

  • garden:ubuntu-25.10-64:tests/main/apparmor-prompting-support

@olivercalder olivercalder left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks, these changes look good! Running a quick rg, I see several other places in the codebase where os.OpenFile, write, then close is used. Sometimes it's for good reason, like appending or writing at a particular offset, but I think there are other cases where we could replace it with os.WriteFile. This would probably be a good task to practice throwing a cheap and/or local LLM at, see what it can find.

@natibek

natibek commented May 1, 2026

Copy link
Copy Markdown
Contributor Author

Thanks! I noticed that too but I thought we wanted to limit it to just the package. Definitely the most LLM friendly task.

@natibek
natibek force-pushed the use-WriteFile-in-apparmor-test branch from 2c460ad to 858b2ef Compare May 5, 2026 19:04
@natibek
natibek requested a review from olivercalder May 5, 2026 19:31

@olivercalder olivercalder left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks good! I went rging for some more potential cases:

snap/squashfs/delta_test.go
721:	f, err := os.OpenFile(deltaPath, os.O_APPEND|os.O_WRONLY, 0644)
722-	c.Assert(err, IsNil)
723-	_, err = f.Write(expectedData)
724-	f.Close()
725-	c.Assert(err, IsNil)
726-
--
829:	f, err := os.OpenFile(deltaPath, os.O_RDWR, 0644)
830-	c.Assert(err, IsNil)
831-	_, err = f.WriteAt([]byte{0x63, 0x00}, 6) // 0x0063 = 99
832-	f.Close()
833-	c.Assert(err, IsNil)
834-

osutil/syncdir_test.go
399:	file, err := os.OpenFile(testPath, os.O_RDWR, 0)
400-	c.Assert(err, IsNil)
401-	defer file.Close()

### mayyybe this one, not sure ###
sandbox/cgroup/freezer.go
181:	f, err := os.OpenFile(where, os.O_WRONLY|os.O_TRUNC, 0644)
182-	if err != nil {
183-		return err
184-	}
185-	_, errW := f.Write(data)
186-	errC := f.Close()

@natibek

natibek commented May 6, 2026

Copy link
Copy Markdown
Contributor Author

I found these as well when searching but none of them can be updated.

  • snap/squashfs/delta_test.go: the first case needs to append to the deltaPath file and os.WriteFile truncates. The second case needs to write at an offset which os.WriteFile does not support.
  • osutil/syncdir_test.go: the file needs to be left open for
	fref = osutil.FileReference{Path: testPath}
	_, _, _, err = fref.State()
  • sandbox/cgroup/freezer.go: it needs to throw an error if the file does not exist.

@natibek
natibek requested a review from olivercalder May 6, 2026 03:06
@Rnfudge02

Rnfudge02 commented May 7, 2026

Copy link
Copy Markdown
Contributor

When I used 'rg' I noticed there are a few additional cases, not sure if they would be candidates or not.

  • bootloader/ubootenv/env_test.go 615:617
  • bootloader/ubootenv/env/go 365:369

These two are tagged RDWR but it seems its just the nil check

  • bootloader/lkenv/lkenv.go 348:352
  • image/image_test.go 5646:5648
  • osutil/disks/gpt_test.go - Multiple

The rg expression I used to check is:

rg -U '(?s)os\.OpenFile.*?Close\(\)'

This surfaces a lot more cases than I've stated, but some are clearly not good candidates.

@Rnfudge02 Rnfudge02 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This looks a lot better than the existing syntax, a few other potential cases, but besides that this is looking good!

@natibek

natibek commented May 7, 2026

Copy link
Copy Markdown
Contributor Author

I took a look at these cases and they all do more than just writing with the *File returned by the call to os.OpenFile. They either avoid truncating, use the file pointer in another function call, write at an offset, or check if the file is created already.

@natibek
natibek requested a review from Rnfudge02 May 7, 2026 13:53

@Rnfudge02 Rnfudge02 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM, thanks!

@olivercalder olivercalder left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks!

@olivercalder
olivercalder merged commit 1d938c0 into canonical:master May 8, 2026
116 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants