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.

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

test: remove hardcoded files to check #39

Merged
merged 1 commit into from
Feb 21, 2024

Conversation

crazy-max
Copy link
Member

Removes hardcoded files to verify in our tests but also check if unexpected files are generated and fail if not available in fixtures.

@crazy-max crazy-max requested a review from thaJeztah June 27, 2023 17:50
@crazy-max crazy-max force-pushed the hardened-tests branch 2 times, most recently from 2272c56 to cb09296 Compare June 27, 2023 17:53
@codecov-commenter
Copy link

codecov-commenter commented Jun 27, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (e1a3957) 67.36% compared to head (04f2e85) 69.58%.
Report is 6 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #39      +/-   ##
==========================================
+ Coverage   67.36%   69.58%   +2.22%     
==========================================
  Files           4        4              
  Lines         576      434     -142     
==========================================
- Hits          388      302      -86     
+ Misses        132       76      -56     
  Partials       56       56              

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

@crazy-max
Copy link
Member Author

@thaJeztah PTAL

clidocstool_md_test.go Outdated Show resolved Hide resolved
bres, err := os.ReadFile(filepath.Join(tmpdir, tt))
seen := make(map[string]struct{})

require.NoError(t, filepath.Walk("fixtures", func(path string, info fs.FileInfo, err error) error {
Copy link
Member

Choose a reason for hiding this comment

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

Can we use filepath.WalkDir for these? Not sure if we need the added features of Walk 🤔

I'm wondering though if we're over-complicating things here; are we expecting to recurse here? Could we use a filepath.Glob(path/*.md) to get the list of markdown files? https://pkg.go.dev/path/filepath#Glob

Copy link
Member Author

Choose a reason for hiding this comment

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

Can we use filepath.WalkDir for these? Not sure if we need the added features of Walk 🤔

I think it's fine to keep Walk even if it's always calling os.Lstat for these test cases. We have less than 10 files.

Could we use a filepath.Glob(path/*.md) to get the list of markdown files? https://pkg.go.dev/path/filepath#Glob

No because we also need to check for dir existence which Walk provides efficiently.

clidocstool_test.go Show resolved Hide resolved
Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

LGTM

@crazy-max crazy-max merged commit 522da51 into docker:main Feb 21, 2024
3 checks passed
@crazy-max crazy-max deleted the hardened-tests branch February 21, 2024 09:32
@crazy-max crazy-max mentioned this pull request Feb 21, 2024
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