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

test(initrd): assert contents of CPIO archive #953

Merged
merged 1 commit into from
Oct 31, 2023
Merged

test(initrd): assert contents of CPIO archive #953

merged 1 commit into from
Oct 31, 2023

Conversation

antoineco
Copy link
Contributor

Prerequisite checklist

  • Read the contribution guidelines regarding submitting new changes to the project;
  • Tested your changes against relevant architectures and platforms;
  • Ran make fmt on your commit series before opening this PR;
  • Updated relevant documentation.

Description of changes

Asserts that CPIO archives are created correctly.

@antoineco antoineco force-pushed the test/initrd branch 2 times, most recently from cacd442 to 32cedbd Compare October 30, 2023 16:07
craciunoiuc
craciunoiuc requested changes Oct 31, 2023
View reviewed changes
initrd/directory_test.go
)

func TestNewFromDirectory(t *testing.T) {
const rootDir = "testdata/rootfs"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const rootDir = "testdata/rootfs"
const rootDir = filepath.Join("testdata", "rootfs")

initrd/directory_test.go
r := cpio.NewReader(openFile(t, irdPath))

expectHeaders := map[string]cpio.Header{
"/entrypoint.sh": {
Copy link
Member

Choose a reason for hiding this comment

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

Not sure how the reader handles paths on Windows, I guess we can leave like this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you have a chance to go test ./initrd since you have Go already set up on Windows?

Copy link
Member

Choose a reason for hiding this comment

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

sure thing

Copy link
Member

Choose a reason for hiding this comment

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

[...]
go: downloading golang.org/x/text v0.12.0
--- FAIL: TestNewFromDirectory (0.02s)
    directory_test.go:83: Encountered unexpected file in cpio archive: testdata/rootfs
    directory_test.go:83: Encountered unexpected file in cpio archive: \entrypoint.sh
    directory_test.go:83: Encountered unexpected file in cpio archive: \etc
    directory_test.go:83: Encountered unexpected file in cpio archive: \etc\app.conf
    directory_test.go:83: Encountered unexpected file in cpio archive: \lib
    directory_test.go:83: Encountered unexpected file in cpio archive: \lib\libtest.so.1
    directory_test.go:83: Encountered unexpected file in cpio archive: \lib\libtest.so.1.0.0
    directory_test.go:34: Failed to remove initrd file D:\Temporary\2117131240: remove D:\Temporary\2117131240: The process cannot access the file because it is being used by another process.
FAIL
FAIL    kraftkit.sh/initrd      0.425s
FAIL

Copy link
Member

Choose a reason for hiding this comment

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

This line Failed to remove initrd file D:\Temporary\2117131240: remove D:\Temporary\2117131240: The process cannot access the file because it is being used by another process. means that os.Remove() was called before all references to the file were closed

Copy link
Member

Choose a reason for hiding this comment

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

PS C:\Users\Cezar\kraftkit> gh pr checkout 953 --force
remote: Enumerating objects: 4, done.
remote: Counting objects: 100% (4/4), done.
remote: Compressing objects: 100% (2/2), done.
remote: Total 4 (delta 2), reused 4 (delta 2), pack-reused 0
Unpacking objects: 100% (4/4), 1.63 KiB | 104.00 KiB/s, done.
From https://github.com/unikraft/kraftkit
 * branch            refs/pull/953/head -> FETCH_HEAD
HEAD is now at 0edcb4d test(initrd): assert contents of CPIO archive
PS C:\Users\Cezar\kraftkit> go test ./initrd
--- FAIL: TestNewFromDirectory (0.01s)
    directory_test.go:83: Encountered unexpected file in cpio archive: testdata/rootfs
    directory_test.go:83: Encountered unexpected file in cpio archive: \entrypoint.sh
    directory_test.go:83: Encountered unexpected file in cpio archive: \etc
    directory_test.go:83: Encountered unexpected file in cpio archive: \etc\app.conf
    directory_test.go:83: Encountered unexpected file in cpio archive: \lib
    directory_test.go:83: Encountered unexpected file in cpio archive: \lib\libtest.so.1
    directory_test.go:83: Encountered unexpected file in cpio archive: \lib\libtest.so.1.0.0
    directory_test.go:34: Failed to remove initrd file: remove D:\Temporary\1822746801: The process cannot access the file because it is being used by another process.
FAIL
FAIL    kraftkit.sh/initrd      0.407s
FAIL

Copy link
Member

Choose a reason for hiding this comment

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

are you sure these are defers though, it uses t.Cleanup which maybe has different functionality?

Copy link
Contributor Author

@antoineco antoineco Oct 31, 2023 •
edited

Choose a reason for hiding this comment

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

Cleanup works the same as defer, except they are scheduled for execution at the end of the test, as opposed to the end of a function.

It would fail in the CI (on Linux) if the code was problematic, because on Linux you also need to call f.Close() otherwise you get the same error. Yet, it fails only on Windows.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Those errors are not checked in Kraftkit:

kraftkit/initrd/directory.go

Lines 68 to 71 in 0d09e39

defer f.Close()
writer := cpio.NewWriter(f)
defer writer.Close()

For all that I know, the problem could be there.

Copy link
Member

Choose a reason for hiding this comment

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

definitely possible 🤷

@antoineco
test(initrd): assert contents of CPIO archive
0edcb4d 
Signed-off-by: Antoine Cotten <antoine@unikraft.io>
craciunoiuc
craciunoiuc approved these changes Oct 31, 2023
View reviewed changes
Copy link
Member

@craciunoiuc craciunoiuc left a comment

Choose a reason for hiding this comment

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

I will merge as is and we (or @nderjung) come back and fix the 2 problems later

Reviewed-by: Cezar Craciunoiu cezar.craciunoiu@unikraft.io
Approved-by: Cezar Craciunoiu cezar.craciunoiu@unikraft.io

@craciunoiuc craciunoiuc merged commit 712c846 into unikraft:staging Oct 31, 2023
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@craciunoiuc craciunoiuc craciunoiuc approved these changes

Assignees
No one assigned
Labels
None yet
Projects
KraftKit Roadmap
Status: 🚀 Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@antoineco @craciunoiuc