Code Coverage: Infiniband unit tests

[ishkool]

Details

Do not mention proprietary info or link to internal work items in this PR.

Work item: _"Internal"

What were the changes?
Adding tests for infiniband network functions.

Why were the changes made?
For improving the code coverage.
Added 12 tests for uncovered function, the functional code coverage is 97.6% (84/86) from previous 83%(72/86)

How was the outcome achieved?
Technical details behind the work. Explain any publicly-available hardware peculiarities.

Additional Documentation:
What else should the reviewer know?

Approval Checklist

Do not approve until these items are satisfied.

• Verify the CHANGELOG has been updated, if
  • there are any NCCL API version changes,
  • any changes impact library users, and/or
  • any changes impact any other ROCm library.

[venksubramd]

TEST(IbTest, UpdateGidIndex)
Please look into this test to see if we can add some asserts and also if it will be useful to split this test into multiple tests.

TEST(IbTest, GidAddrPrefix)
We have an assert for true, do we need a test for assert of false return value? Also, do we need more to test the actual behavior of this function?

TEST(IbTest, AddrRange)
Do we need to check the value assigned to mask? Other behavior of this function?

TEST(IbTest, Devftl)
This test looks like sufficient for this function.

TEST(IbTest, EnvAddrFamily)
Do we need more tests for other returned values for the res, like AF_INET6?

TEST(IbTest, GetGidAddrFamily)
Do we need more tests for other returned values for the res?

TEST(IbTest, ConfiguredGid)
Do we need a test for the false return value?

TEST(IbTest, LinkLocalGid)
Please include assert so test indicates fail if unsuccessful.

TEST(IbTest, ValidGid)
Please include assert so test indicates fail if unsuccessful.

TEST(IbTest, RoceVer)
Do we need to verify behavior of this function in addition to checking the return value?
What about situations that return something other than ncclSuccess, like error for instance?

[ishkool]

TEST(IbTest, UpdateGidIndex) Please look into this test to see if we can add some asserts and also if it will be useful to split this test into multiple tests.

Actually, all functions used by this test have their corresponding tests in this file I'll add asserts when they are called by this function.

TEST(IbTest, GidAddrPrefix) We have an assert for true, do we need a test for assert of false return value? Also, do we need more to test the actual behavior of this function?

Function will return false if perfix does not match, we are providing dummy value such that it matches hence the assert on true. Also function does most of work while matching so that is targeted as well.

TEST(IbTest, AddrRange) Do we need to check the value assigned to mask? Other behavior of this function?

Not needed we just want the pointer to not be null, mask is based on the input.

TEST(IbTest, Devftl) This test looks like sufficient for this function.

Yes cause it just does an atomic addition operation over a struct variable.

TEST(IbTest, EnvAddrFamily) Do we need more tests for other returned values for the res, like AF_INET6?

Thanks I'll check if ipv6 is supported which most probably will be and will modify accordingly.
Update: It's an env variable which defaults to ipv4 so not changing anything here.

TEST(IbTest, GetGidAddrFamily) Do we need more tests for other returned values for the res?

It just returns which prortocal we use 2 is for AF_INET IPV4 , I think we support ipv4 and ipv6 qill check that.

TEST(IbTest, ConfiguredGid) Do we need a test for the false return value?

I don't think so, what you want to configure should be successful unless inputs are bad, and we are already using correct inps in the dummy vals.

TEST(IbTest, LinkLocalGid) Please include assert so test indicates fail if unsuccessful.

Thanks, will do.

TEST(IbTest, ValidGid) Please include assert so test indicates fail if unsuccessful.

Thanks, will do.

TEST(IbTest, RoceVer) Do we need to verify behavior of this function in addition to checking the return value? What about situations that return something other than ncclSuccess, like error for instance?

If error is returned then Roce is not supported maybe.

[rahulvaidya20]

Some of the tests seem to only validate "happy path" scenarios. We also need to add more tests to cover error paths and other edge cases to make sure the functions don't break. This can be addressed in a separate PR.

[venksubramd]

We could create another PR for the cases that @rahulvaidya20 has suggested and close this one.