Fix flaky unit Test_getCompleteCustomPortPairs #6737
Conversation
Signed-off-by: Parthvi Vala <pvala@redhat.com>
✅ Deploy Preview for odo-docusaurus-preview canceled.
|
openshift-ci
bot
added
area/testing
| @@ -23,6 +23,7 @@ func Test_getCompleteCustomPortPairs(t *testing.T) { | |||
| args: args{ | |||
| definedPorts: []api.ForwardedPort{ | |||
| {ContainerName: "runtime", LocalPort: 8080, ContainerPort: 8000}, | |||
| {ContainerName: "tools", LocalPort: 5000, ContainerPort: 5000}, | |||
There was a problem hiding this comment.
Previously, there were 2 non-customized ports, and a random value was assigned to 5000 and 9000 from time to time, sometimes 20001 would be assigned to 5000 and 20002 to 9000 and vice versa.
Adding 5000 to the definedPorts will help avoid this flake while also maintaining the test integrity.
|
OC Failure: [FAILED] Timed out after 180.000s.
Expected process to exit. It did not.
In [AfterEach] at: /go/odo_1/tests/helper/helper_dev.go:211 @ 04/14/23 05:41:57.43
------------------------------
Summarizing 1 Failure:
[FAIL] odo dev command tests when Starting a PostgreSQL service when creating local files and dir and running odo dev - without metadata.name [AfterEach] when deleting local files and dir and waiting for sync should not list deleted dir and file in container
/go/odo_1/tests/helper/helper_dev.go:211Windows Failure: [FAILED] No future change is possible. Bailing out early after 1.332s.
Running oc.exe with args [oc get pods -n interactive-add-binding-test577amu -o jsonpath='{range .items[*]}{.metadata.name}'] and odo env: []
Expected
<int>: 1
to match exit code:
<int>: 0
[FAILED] No future change is possible. Bailing out early after 1.192s.
Running oc.exe with args [oc exec nxbmst-app-7b69cff74b-9m7wl --namespace cmd-dev-test1738lfs -- ls -lai /projects/webapp] and odo env: []
Expected
<int>: 1
to match exit code:
<int>: 0
[FAILED] No future change is possible. Bailing out early after 21.166s.
Running oc.exe with args [oc create configmap config-map-for-cleanup --from-literal type=testing --from-literal team=odo -n cmd-delete-test625lsx] and odo env: []
Expected
<int>: 1
to match exit code:
<int>: 0
In [BeforeEach] at: C:/Users/Administrator.ANSIBLE-TEST-VS/3699/tests/helper/helper_cmd_wrapper.go:101 @ 04/14/23 00:19:16.65
Summarizing 3 Failures:
[FAIL] odo delete command tests [BeforeEach] when a component is bootstrapped when the component is running in both DEV and DEPLOY mode and dev mode is killed when the component is deleted while having access to the devfile.yaml should delete the appropriate resources
C:/Users/Administrator.ANSIBLE-TEST-VS/3699/tests/helper/helper_cmd_wrapper.go:101
[FAIL] odo add binding interactive command tests [BeforeEach] when running a deployment when binding to a service in a different namespace should error out if service is not found in the namespace selected
C:/Users/Administrator.ANSIBLE-TEST-VS/3699/tests/helper/helper_generic.go:58
[FAIL] odo dev command tests when multiple projects are present - with metadata.name [It] should sync to the correct dir in container
C:/Users/Administrator.ANSIBLE-TEST-VS/3699/tests/helper/helper_cmd_wrapper.go:101
Ran 437 of 805 Specs in 1189.646 seconds
FAIL! -- 434 Passed | 3 Failed | 0 Pending | 368 Skipped |
There was a problem hiding this comment.
I noticed that all the test cases here are relying on opening actual ports and expecting exact ports. But if I already have some other apps listening on those ports, then the test would fail.
I ran into this issue because I had unrelated odo dev sessions running and taking ports 20001 through 20003, and the test is now failing:
--- FAIL: Test_getCompleteCustomPortPairs (0.00s)
--- FAIL: Test_getCompleteCustomPortPairs/ports_are_provided_with_container_name (0.00s)
portForward_test.go:78: getCompleteCustomPortPairs() (got vs want) diff = map[string][]string{
"runtime": {
"8080:8000",
- "20004:9000",
+ "20001:9000",
},
"tools": {"5000:5000"},
}
--- FAIL: Test_getCompleteCustomPortPairs/ports_are_provided_without_container_name (0.00s)
portForward_test.go:78: getCompleteCustomPortPairs() (got vs want) diff = map[string][]string{
"runtime": {
"8080:8000",
- "20004:9000",
+ "20001:9000",
},
"tools": {"5000:5000"},
}
--- FAIL: Test_getCompleteCustomPortPairs/local_ports_in_range_[20001-30001]_are_provided_as_custom_forward_ports (0.00s)
portForward_test.go:78: getCompleteCustomPortPairs() (got vs want) diff = map[string][]string{
"runtime": {"20001:8000", "20002:9000"},
"tools": {
"5000:5000",
- "20004:8080",
+ "20003:8080",
},
}
Ideally, the unit test should not have to open any actual port, but I understand this is exactly what we are trying to test here. So I'd suggest relaxing the checks, by just checking that the port pairs we are getting in response are just within the range we expect. What do you think?
There was a problem hiding this comment.
I noticed that all the test cases here are relying on opening actual ports and expecting exact ports. But if I already have some other apps listening on those ports, then the test would fail. I ran into this issue because I had unrelated
odo devsessions running and taking ports 20001 through 20003, and the test is now failing:--- FAIL: Test_getCompleteCustomPortPairs (0.00s) --- FAIL: Test_getCompleteCustomPortPairs/ports_are_provided_with_container_name (0.00s) portForward_test.go:78: getCompleteCustomPortPairs() (got vs want) diff = map[string][]string{ "runtime": { "8080:8000", - "20004:9000", + "20001:9000", }, "tools": {"5000:5000"}, } --- FAIL: Test_getCompleteCustomPortPairs/ports_are_provided_without_container_name (0.00s) portForward_test.go:78: getCompleteCustomPortPairs() (got vs want) diff = map[string][]string{ "runtime": { "8080:8000", - "20004:9000", + "20001:9000", }, "tools": {"5000:5000"}, } --- FAIL: Test_getCompleteCustomPortPairs/local_ports_in_range_[20001-30001]_are_provided_as_custom_forward_ports (0.00s) portForward_test.go:78: getCompleteCustomPortPairs() (got vs want) diff = map[string][]string{ "runtime": {"20001:8000", "20002:9000"}, "tools": { "5000:5000", - "20004:8080", + "20003:8080", }, }Ideally, the unit test should not have to open any actual port, but I understand this is exactly what we are trying to test here. So I'd suggest relaxing the checks, by just checking that the port pairs we are getting in response are just within the range we expect. What do you think?
In the other hand, this solution will hide the problem raised in #6741: I think we want to have the same local ports every time we work on a Devfile with several ports to forward. Without fixing
#6741, local ports will differ from one run to another.
There was a problem hiding this comment.
I think we want to have the same local ports every time we work on a Devfile with several ports to forward.
We do, but that is why we are defining custom port mapping. For ports that we do not define a custom mapping, there will always be a chance that you get a different port number every time, so perhaps it does make sense that we only check the port range instead of the exact value. Although we still want to ensure it does not assign the same port number more than once, I can add an additional check for that.
Regardless of how we test, I agree that we can still loop over the containers in an orderly manner.
There was a problem hiding this comment.
In the other hand, this solution will hide the problem raised in #6741: I think we want to have the same local ports every time we work on a Devfile with several ports to forward. Without fixing
#6741, local ports will differ from one run to another.
I agree with ensuring we have the same local ports. As Parthvi said, that should be okay, I think, for custom ports.
Overall, my main concern is more on how those unit tests are depending on the ports opened on the system.. So I'd need to make sure I don't have any other unrelated application listening on the local ports expected..
There was a problem hiding this comment.
FYI, I reverted the entire commit (fb31c33), which means current files do not contain the relaxed checks. I shall revert it again once we have a consensus.
There was a problem hiding this comment.
Whoops, just noticed this PR got merged already. Alright, let's see if this becomes annoying on a daily basis..
Signed-off-by: Parthvi Vala <pvala@redhat.com>
This reverts commit 1bcbacf.
Signed-off-by: Parthvi Vala <pvala@redhat.com>
|
Kudos, SonarCloud Quality Gate passed!
|
|
/override windows-integration-test/Windows-test |
|
@valaparthvi: Overrode contexts on behalf of valaparthvi: windows-integration-test/Windows-test In response to this:
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. |
|
/lgtm |
openshift-ci
bot
added
the
lgtm
What type of PR is this:
/area testing
/kind flake
What does this PR do / why we need it:
This fixes flaky test.
Which issue(s) this PR fixes:
Fixes part of #6479
Fixes #6741
PR acceptance criteria:
Unit test
Integration test
Documentation
How to test changes / Special notes to the reviewer: