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

Fix flaky unit Test_getCompleteCustomPortPairs #6737

Merged
merged 4 commits into from
Apr 18, 2023
Merged

Fix flaky unit Test_getCompleteCustomPortPairs #6737

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
ddad1dc
Fix flaky unit Test_getCompleteCustomPortPairs
valaparthvi Apr 13, 2023
1bcbacf
Use range check
valaparthvi Apr 17, 2023
fb31c33
Revert "Use range check"
valaparthvi Apr 17, 2023
5afb86b
Use order to iterate over ceMapping
valaparthvi Apr 17, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion pkg/portForward/kubeportforward/portForward_test.go
View file
Open in desktop
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

Whoops, just noticed this PR got merged already. Alright, let's see if this becomes annoying on a daily basis..

Original file line number Diff line number Diff line change
Expand Up @@ -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},
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

},
ceMapping: map[string][]v1alpha2.Endpoint{
"runtime": {{TargetPort: 8000}, {TargetPort: 9000}},
Expand All @@ -31,7 +32,7 @@ func Test_getCompleteCustomPortPairs(t *testing.T) {
},
wantPortPairs: map[string][]string{
"runtime": {"8080:8000", "20001:9000"},
"tools": {"20002:5000"},
"tools": {"5000:5000"},
},
},
{
Expand Down