Skip to content
This repository has been archived by the owner on May 12, 2021. It is now read-only.

Unit test: fix bugs on a few unit tests on aarch64 #1202

Merged
merged 6 commits into from
Feb 15, 2019
Merged

Unit test: fix bugs on a few unit tests on aarch64 #1202

merged 6 commits into from
Feb 15, 2019

Conversation

Pennyzct
Copy link
Contributor

@Pennyzct Pennyzct commented Jan 31, 2019 •
edited
Loading

Description of problem

09:55:00 cli/kata-check_arm64.go:129:9:warning: if block ends with a return statement, so drop this else and outdent its block (move short variable declaration to its own line if necessary) (golint)
09:55:02 virtcontainers/qemu_arm64_test.go:43:23:warning: unused variable or constant undeclared name: virt (varcheck)
09:55:02 virtcontainers/qemu_arm64_test.go:50:11:warning: unused variable or constant undeclared name: defaultMemSlots (varcheck)
09:55:02 virtcontainers/qemu_arm64_test.go:54:40:warning: unused variable or constant too few arguments in call to arm64.memoryTopology (varcheck)
09:55:02 virtcontainers/qemu_arm64_test.go:29:23:warning: unused variable or constant undeclared name: virt (varcheck)
09:55:02 cli/kata-check_test.go:141:113:warning: unused variable or constant undeclared name: TestDataa (varcheck)
09:55:03 virtcontainers/qemu_arm64_test.go:43:23:warning: unused struct field undeclared name: virt (structcheck)
09:55:03 virtcontainers/qemu_arm64_test.go:50:11:warning: unused struct field undeclared name: defaultMemSlots (structcheck)
09:55:03 virtcontainers/qemu_arm64_test.go:54:40:warning: unused struct field too few arguments in call to arm64.memoryTopology (structcheck)
09:55:03 virtcontainers/qemu_arm64_test.go:29:23:warning: unused struct field undeclared name: virt (structcheck)
09:55:03 cli/kata-check_test.go:141:113:warning: unused struct field undeclared name: TestDataa (structcheck)
09:55:03 cli/kata-check_test.go:141:113:warning: undeclared name: TestDataa (unused)
09:55:04 cli/kata-check_test.go:141:113:warning: undeclared name: TestDataa (staticcheck)
09:55:05 Build step 'Execute shell' marked build as failure

There existed some bugs on a few unit tests on aarch64, and I will fix them one by one.

@jodh-intel
Copy link
Contributor

Hi @Pennyzct - The ppc64le-specific Travis build is still complaining about a couple of issues:

virtcontainers/hypervisor_test.go:440:6:warning: type testNestedVMMData is unused (U1000) (unused)
virtcontainers/hypervisor_test.go:446:6:warning: func genericTestRunningOnVMM is unused (U1000) (unused)

@Pennyzct
Copy link
Contributor Author

Hi~ @jodh-intel I'm kind of confused. Those generic* functions are like a template for all arch. you can choose to use or do your own arch-specific implementation, just like func genericMemoryTopology. I didn't do the work for ppc64le, but it share the same logic with aarch64. That is, it also couldn't use the genericTestRunningOnVMM and need to implement its own function.

@jodh-intel
Copy link
Contributor

Hi @Pennyzct - it confused me a bit. Maybe @nitkon could tal?

@Pennyzct Pennyzct mentioned this pull request Feb 1, 2019
Merged
@raravena80
Copy link
Member

@Pennyzct @nitkon ping, any updates? Thx!

This was referenced Feb 7, 2019
Closed
Closed
Closed
@Pennyzct
Copy link
Contributor Author

/retest

@sameo sameo mentioned this pull request Feb 11, 2019
Closed
@Pennyzct
Copy link
Contributor Author

Hi~ @jodh-intel @grahamwhaley @sameo Do we need to put // nolint: varcheck,unused, // nolint: unused on all generic var and function?
All existing PR including #1218, #1223 are trying to fix the unused statis error, and here, the travis-ci failing is also because that newly implemented generic var testNestedVMMData and function genericTestRunningOnVMM is not used in ppc64le.

This was referenced Feb 11, 2019
Merged
Merged
@Pennyzct Pennyzct force-pushed the unit-test branch 2 times, most recently from 487ea1c to 389495a Compare February 13, 2019 02:45
@Pennyzct
Copy link
Contributor Author

/retest

@Pennyzct Pennyzct force-pushed the unit-test branch 2 times, most recently from e670203 to 70a719a Compare February 13, 2019 07:24
@Pennyzct
Copy link
Contributor Author

/retest

@Pennyzct
Copy link
Contributor Author

/retest

@Pennyzct
Copy link
Contributor Author

Pennyzct commented Feb 13, 2019 •
edited
Loading

Hi~ @jodh-intel @grahamwhaley @sameo @teawater all former static check errors haven been eliminated.
For now, it stuck here.

17:15:07 === RUN   TestCreateEmptySandbox
17:15:07 time="2019-02-13T17:15:06+08:00" level=error msg="Create new sandbox failed" error="Missing kernel path" sandbox=7f49d00d-1995-4156-8c79-5f5ab24ce138 sandboxid=7f49d00d-1995-4156-8c79-5f5ab24ce138 source=virtcontainers subsystem=sandbox
17:15:07 time="2019-02-13T17:15:06+08:00" level=error msg="Create new sandbox failed" error="Missing kernel path" sandbox=7f49d00d-1995-4156-8c79-5f5ab24ce138 sandboxid=7f49d00d-1995-4156-8c79-5f5ab24ce138 source=virtcontainers subsystem=sandbox
17:15:07 time="2019-02-13T17:15:07+08:00" level=error msg="failed to start proxy" error="Invalid proxy parameters {id:foobar path: agentURL: consoleURL: logger:0x40007c86e0 debug:false}" params="{foobar    0x40007c86e0 false}" proxy type=invalidProxyType source=virtcontainers vm=foobar
17:15:07 FAIL	github.com/kata-containers/runtime/virtcontainers	4.690s

I will /retest to see if it reproduces itself.

@Pennyzct
Copy link
Contributor Author

/retest

@Pennyzct
unit-test: struct TestDataa should be included in arch-indenpedent .g…
44e2b9a 
…o file

argument struct TestDataa in generic func genericTestGetCPUDetails is repeatedly
defined in almost all arch-dependent .go file, cli/kata-check_amd64_test.go,
cli/kata-check_ppc64le_test.go, etcm, except arm64. let's only declare it once in
cli/kata-check_test.go. change its name to testCPUDetail for better understanding.

Fixes: kata-containers#1200

Signed-off-by: Penny Zheng <[email protected]>
@Pennyzct
unit-test: refine qemu_arm64_test.go
0679f6f 
refine a set of test functions under qemu_arm64_test.go. e.g. test
func for memoryTopology shouldn't be the same one on amd64, since
for now, we don't support nvdimm on arm64.

Fixes: kata-containers#1200

Signed-off-by: Penny Zheng <[email protected]>
@Pennyzct
Copy link
Contributor Author

/retest

@Pennyzct
unit-test: test func for RunningOnVMM should be arch-dependent
e93fb0b 
original tests for func RunningOnVMM are sort of amd64-specific,
since all other archs don't support nested VMM for now.

Fixes: kata-containers#1200

Signed-off-by: Penny Zheng <[email protected]>
@Pennyzct
runtime: add appendBridges for arm64
3ec56ea 
since generic func genericAppendBridges and genericBridges
is also applied for machine type QemuVirt, we use it as implementation
for appendBridges and bridges on aarch64.
since const defaultPCBridgeBus is used in generic func
genericAppendBridges for pc machine, we should define it once
in generic file, instead of redefining it in different
arch-specific files.

Fixes: kata-containers#1200

Signed-off-by: Penny Zheng <[email protected]>
@Pennyzct
unit-test: add nolint comment to avoid unused warning
1b967a4 
since all generic* could bring unused linter warnings, which lead to
CI crash, we add nolint comment to avoid them.

Fixes: kata-containers#1200

Signed-off-by: Samuel Ortiz <[email protected]>
Signed-off-by: Penny Zheng <[email protected]>
@Pennyzct
Copy link
Contributor Author

/retest

@Pennyzct
unit-test: refine func TestGetCPUDetails
23c554e 
refine struct testData in func TestGetCPUDetails to remove redundant
/unused struct field expectedVendor and expectedModel

Fixes: kata-containers#1200

Signed-off-by: Penny Zheng <[email protected]>
@Pennyzct
Copy link
Contributor Author

/retest

@Pennyzct
Copy link
Contributor Author

Hi~ @jodh-intel @grahamwhaley @sameo finally, i got almost all green on this PR, especially, ARM CI has entirely passed all tests. ;). ptal.

@chavafg
Copy link
Contributor

chavafg commented Feb 14, 2019

I see that the vsock job failed 2 times on the cpu hot-plug tests, but your changes seem unrelated:

Summarizing 2 Failures:

[Fail] Hot plug CPUs container with CPU period and quota [It] should have 3 CPUs 
/tmp/jenkins/workspace/kata-containers-runtime-fedora-vsocks-PR/go/src/github.com/kata-containers/tests/integration/docker/cpu_test.go:89

[Fail] Hot plug CPUs container with CPU constraint [It] should have 2 CPUs 
/tmp/jenkins/workspace/kata-containers-runtime-fedora-vsocks-PR/go/src/github.com/kata-containers/tests/integration/docker/cpu_test.go:108

Ran 225 of 234 Specs in 2520.438 seconds
FAIL! -- 223 Passed | 2 Failed | 0 Pending | 9 Skipped --- FAIL: TestIntegration (2547.13s)
FAIL

http://jenkins.katacontainers.io/job/kata-containers-runtime-fedora-vsocks-PR/217/consoleText
http://jenkins.katacontainers.io/job/kata-containers-runtime-fedora-vsocks-PR/218/console

Any idea @devimc

@Pennyzct
Copy link
Contributor Author

Hi~ I get all green here.;). Could anyone give it a review? thanks a lot! @jodh-intel @grahamwhaley @sameo @chavafg @devimc

grahamwhaley
grahamwhaley approved these changes Feb 15, 2019
View reviewed changes
Copy link
Contributor

@grahamwhaley grahamwhaley left a comment

Choose a reason for hiding this comment

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

nice set of fixes.
I see @sameo SoB on at least one of the patches, so I'd like to see his ack here.

@Pennyzct
Copy link
Contributor Author

one commit unit-test: add nolint comment to avoid unused warning is inspired by his commit virtcontainers: Tag unused functions and routines for ARM64 on #1218, so i hope to add his @sameo SOB. ;)

sameo
sameo approved these changes Feb 15, 2019
View reviewed changes
Copy link

@sameo sameo left a comment

Choose a reason for hiding this comment

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

LGTM

@grahamwhaley grahamwhaley merged commit 816ea42 into kata-containers:master Feb 15, 2019
@egernst egernst mentioned this pull request Feb 26, 2019
Merged
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@grahamwhaley grahamwhaley grahamwhaley approved these changes

@sameo sameo sameo approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@Pennyzct @jodh-intel @raravena80 @chavafg @sameo @grahamwhaley