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

Update (functional) tests directory #1263

Closed
wants to merge 5 commits into from

Conversation

helsaawy
Copy link
Contributor

@helsaawy helsaawy commented Jan 5, 2022

Updated ./test/functional to:

  • use command line arguments to configure tests
  • use ctr.exe to find the layer path for images
  • use a client to interact with containerd directly

Moved out utilities and manifest from ./test/functional into ./test/testutil

  • better delineate between utilities and tests
  • factor out common code needed across functional and cri-containerd tests

Moved schema test from test/internal into internal/schemaversion,
and added test/testutil/manifest as an import dependency to get OS
version. Removed internal from github actions testing

Updated module paths to point to correct test folders

Updated `./tests/functional` to:
- use command line arguments to configure tests
- use `ctr.exe` to find the layer path for images
- better delineate between utilities and test with config
- added client to interact with containerd directly

Signed-off-by: Hamza El-Saawy <hamzaelsaawy@microsoft.com>
Signed-off-by: Hamza El-Saawy <hamzaelsaawy@microsoft.com>
Added in changes from
microsoft@8b80eda

Signed-off-by: Hamza El-Saawy <hamzaelsaawy@microsoft.com>
Moved test utilities from `test/functional/utilities` to `test/testutil`
Moved manifest into new test utilities directory

Moved schema test from `test/internal` into `internal/schemaversion`,
and added `test/testutil/manifest` as an import dependency to get OS
version. Removed internal from github actions testing

Updated module paths to point to correct test folders

Signed-off-by: Hamza El-Saawy <hamzaelsaawy@microsoft.com>
Comment on lines -76 to -77
- run: go test -gcflags=all=-d=checkptr -v ./internal -tags admin
working-directory: test
Copy link
Contributor

Choose a reason for hiding this comment

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

why 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.

The only file in test/internal was a schemaversion test file, which I moved into the code library.
It was only ever in the test directory because it needed a manifest file to get the OS version, but with the manifest being moved out into testutil, it can be imported into unit tests in the main code base without any issues

Comment on lines 94 to 95
func getCtrdOptions() testutil.CtrdClientOptions {
return testutil.CtrdClientOptions{
Copy link
Contributor

Choose a reason for hiding this comment

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

can we just use the word containerd here? Makes the distinction between this and the function above clearer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

}

// CreateWCOWUVMFromOpts creates a WCOW utility VM with the passed opts.
func CreateWCOWUVMFromOpts(ctx context.Context, t *testing.T, _ *containerd.Client, opts *uvm.OptionsWCOW) *uvm.UtilityVM {
Copy link
Contributor

Choose a reason for hiding this comment

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

why do we pass through the client here if it's not used?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Basically CreateWCOWUVMFromOptsWithImage needs it, so I added it to all the functions here to maintain signature consistency, but I can remove it everywhere except for the two functions that need it.

@katiewasnothere
Copy link
Contributor

Why is this a draft PR?

Signed-off-by: Hamza El-Saawy <hamzaelsaawy@microsoft.com>
@helsaawy
Copy link
Contributor Author

helsaawy commented Jan 6, 2022

Why is this a draft PR?

I am not sure if the code restructure will simplify things or make testing more difficult in the future, because of the potentially cyclic relationship between hcsshim tests and the tests dir.
Plus, I will likely start pulling things out of cri-containerd and move them into testutil as I come across them

@katiewasnothere
Copy link
Contributor

@helsaawy could we break anything out of this draft that we definitely want and PR those?

@helsaawy
Copy link
Contributor Author

in favor of #1351 and #1352

@helsaawy helsaawy closed this Aug 26, 2022
@helsaawy helsaawy deleted the he/functests branch September 19, 2022 18:29
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.

None yet

2 participants