For goveralls fix races in unit tests #5262
Closed
Conversation
This file contains 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
|
Skipping CI for Draft Pull Request. |
kubevirt-bot
added
do-not-merge/work-in-progress
|
/test pull-kubevirt-goveralls |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/test pull-kubevirt-unit-test |
|
/test pull-kubevirt-goveralls |
|
This seems like a hard nut to crack. I've done several test runs, and seen sometimes several data races in one run (especially regarding |
|
|
kubevirt-bot
added
the
needs-rebase
dhiller
force-pushed
the
for-goveralls-fix-races-in-unit-tests
branch
from
85e9cfb
to
7957088
Compare
kubevirt-bot
added
needs-rebase
dhiller
force-pushed
the
for-goveralls-fix-races-in-unit-tests
branch
from
7957088
to
252af94
Compare
|
/test pull-kubevirt-unit-test |
dhiller
force-pushed
the
for-goveralls-fix-races-in-unit-tests
branch
from
80c5b2f
to
b541dbe
Compare
kubevirt-bot
removed
the
needs-rebase
|
/test pull-kubevirt-unit-test |
|
kubevirt_test.go data races seem to have gone with the changes. |
|
/test pull-kubevirt-unit-test |
dhiller
force-pushed
the
for-goveralls-fix-races-in-unit-tests
branch
from
8e7c8d7
to
f37f65e
Compare
|
/test pull-kubevirt-unit-test |
|
kubevirt-bot
added
the
needs-rebase
Signed-off-by: Roman Mohr <rmohr@redhat.com>
Signed-off-by: Roman Mohr <rmohr@redhat.com>
Signed-off-by: Roman Mohr <rmohr@redhat.com>
Signed-off-by: Daniel Hiller <dhiller@redhat.com>
Example:
KUBEVIRT_FOCUS="On valid KubeVirt object" \
KUBEVIRT_EXTRA_ARGS="--runs_per_test=5 --test_output=all --cache_test_results=no" \
KUBEVIRT_TARGETS="//pkg/virt-operator:*" make test
Signed-off-by: Daniel Hiller <dhiller@redhat.com>
Create a test data struct in order to separate the required test state (mocks, recording structures etc) from the actual run, so we do not share state between tests anymore. This eliminates the data races. Also add the behavior to the struct so that it uses the test state and not the shared state. Increase timeouts for tests that regularly fail on parallel execution. Signed-off-by: Daniel Hiller <dhiller@redhat.com>
See https://docs.bazel.build/versions/master/test-encyclopedia.html#role-of-the-test-runner Also add the test.log output to the artifacts for having more information. Signed-off-by: Daniel Hiller <dhiller@redhat.com>
Also add ctrl.Finish() and do expect call before changing the mock handler. Signed-off-by: kubevirt-bot <kubevirtbot@redhat.com>
dhiller
force-pushed
the
for-goveralls-fix-races-in-unit-tests
branch
from
e5cfa24
to
c4e7df3
Compare
kubevirt-bot
removed
the
needs-rebase
|
/test pull-kubevirt-unit-test |
dhiller
force-pushed
the
for-goveralls-fix-races-in-unit-tests
branch
from
c4e7df3
to
e9e0f43
Compare
|
/test pull-kubevirt-unit-test |
Also increase go_default_test timeout to "long" for virt-handler tests. Signed-off-by: Daniel Hiller <dhiller@redhat.com>
dhiller
force-pushed
the
for-goveralls-fix-races-in-unit-tests
branch
from
e9e0f43
to
19756a4
Compare
|
Kudos, SonarCloud Quality Gate passed!
|
|
/test pull-kubevirt-unit-test |
|
/test pull-kubevirt-goveralls |
|
testing When investigating I found some strange error compiling Here's the test patch file: https://gist.github.com/dhiller/b51bca27d96b35f27b9fdad8dc30396d Complete error: @rmohr any ideas? |
Signed-off-by: Daniel Hiller <dhiller@redhat.com>
unsafeptr fails in vendored dependencies. Signed-off-by: Daniel Hiller <dhiller@redhat.com>
|
/test pull-kubevirt-unit-test |
|
@dhiller: The following test failed, say
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
kubevirt-bot
added
the
needs-rebase
|
@dhiller: PR needs rebase. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
This was referenced May 3, 2021
Merged
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.
What this PR does / why we need it:
This PR picks up the original fixes for data races from @rmohr .
Furthermore it addresses the data races that are getting visible for
kubevirt_test.gowhen the tests are run in parallel, i.e. when using bazel options like this:--runs_per_test=4 --cache_test_results=noIn that case i.e. several data races got visible. This was addressed by isolating the test data (mocks, recording data structures etc.) into a separate struct that is created per test, in order to isolate data access of each test. Also the function lambdas were attached to the struct to use the data from the test data struct.Finally this PR makes available some of the bazel options so that they can be used when calling
make testExample:
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close the issue(s) when PR gets merged):Fixes #5313
Special notes for your reviewer:
I think we can improve the readability and interface of the created struct in a fluent interface way in order to improve reading the test code. But the changes here are required to enable the goveralls lane, so I would prefer to do this in a second PR.
Example:
Release note: