test(utils): fix tests that assert nothing and run them in CI#1680
Open
ihopenre-eng wants to merge 1 commit into
Open
test(utils): fix tests that assert nothing and run them in CI#1680ihopenre-eng wants to merge 1 commit into
ihopenre-eng wants to merge 1 commit into
Conversation
The utils package is not part of `make test`, which runs `./pkg/...`
only, so its tests never ran in CI. Three of them fail today.
TestValidateFilePaths and TestReadFile create a temp fixture and remove
it with `defer` while their subtests call `t.Parallel()`. A parallel
subtest is paused until its parent returns, so the deferred cleanup
deletes the fixture before any subtest reads it, and the "valid file
path" cases assert an error that only proves the file is gone. This
fails on every platform:
--- FAIL: TestValidateFilePaths (0.01s)
--- FAIL: TestReadFile (0.00s)
Move both to t.Cleanup, which runs after parallel subtests finish, the
pattern TestFindFileInDir in this same file already uses.
TestGetAbsPath additionally fails on Windows: "/absolute/path" has no
volume name, so filepath.IsAbs reports false for it and GetAbsPath joins
it with baseDir. Pick the absolute path per platform.
Add ./utils/... to the test target so these stay honest.
Signed-off-by: ihopenre-eng <247072151+ihopenre-eng@users.noreply.github.com>
Author
|
The CI workflows on this PR are queued at Could a maintainer approve the workflow runs when convenient? Happy to rebase if anything has drifted in the meantime. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
The
utilspackage is not part ofmake test, which runs./pkg/...only, so its tests never ran in CI. Three of them fail today on a clean master.TestValidateFilePathsandTestReadFilecreate a temp fixture and remove it withdeferwhile their subtests callt.Parallel(). A parallel subtest is paused until its parent function returns, so the deferred cleanup deletes the fixture before any subtest reads it, and the "valid file path" cases assert an error that only proves the file is already gone. They fail on Linux and Windows alike. Both move tot.Cleanup, which runs after parallel subtests finish, matching whatTestFindFileInDirin the same file already does.TestGetAbsPathfails on Windows only:/absolute/pathcarries no volume name, sofilepath.IsAbsreports false for it andGetAbsPathjoins it withbaseDir. The absolute path is now chosen per platform../utils/...is added to thetesttarget so these keep working.Before:
After, on windows/amd64 and on linux/amd64 (cross-compiled test binary run under WSL):
No production code is touched.
Issue reference
Please reference the issue this PR will close: #1679
Checklist
Please make sure you've completed the relevant tasks for this PR, out of the following list: