TL/MLX5: add gtest for mcast

[MamziB]

TL/MLX5: add gtest for mcast

[samnordmann]

I don't think those tests are run... On both Azure and github CI, tl/mlx5/bcast seem to have trouble initializing. Take a look at the logs

[MamziB]

@samnordmann thanks Sam for the comments. I updated the PR

[samnordmann]

I still think that those tests are not run. Am I missing something?

[MamziB]

So we have multiple algorithms in this file for Bcast, I added MCAST as new algorithm to the list. Not sure what else I should do.

[samnordmann]

Can you please re-trigger the CI?

[MamziB]

@samnordmann done

[nsarka]

This is probably not a change for this PR, but should we update the gtest to show which tests are run instead of just a number?

For example,

diff --git a/test/gtest/coll/test_bcast.cc b/test/gtest/coll/test_bcast.cc
index 6d80816..faf4318 100644
--- a/test/gtest/coll/test_bcast.cc
+++ b/test/gtest/coll/test_bcast.cc
@@ -276,7 +276,20 @@ ucc_job_env_t two_step_env = {{"UCC_CL_HIER_TUNE", "bcast:@2step:0-inf:inf"},
                               {"UCC_CLS", "all"}};
 ucc_job_env_t dbt_env      = {{"UCC_TL_UCP_TUNE", "bcast:@dbt:0-inf:inf"},
                               {"UCC_CLS", "basic"}};
-INSTANTIATE_TEST_CASE_P(
+
+struct PrintToStringParamNameEdited {
+  static bool remove_non_alphanumeric(char c) {
+    return !isalnum(c) && c != '_';
+  }
+  template <class Param_2>
+  std::string operator()(const testing::TestParamInfo<Param_2>& info) const {
+    std::string s = ::testing::PrintToString(info.param);
+    s.erase(std::remove_if(s.begin(), s.end(), remove_non_alphanumeric), s.end());
+    return s;
+  }
+};
+
+INSTANTIATE_TEST_SUITE_P(
     , test_bcast_alg,
     ::testing::Combine(
 #ifdef HAVE_CUDA
@@ -287,4 +300,4 @@ INSTANTIATE_TEST_CASE_P(
 #endif
         ::testing::Values(two_step_env, dbt_env), //env
         ::testing::Values(8, 65536), // count
-        ::testing::Values(15,16))); // n_procs
+        ::testing::Values(15,16)), PrintToStringParamNameEdited()); // n_procs

This will make it so instead of the test run looking like:

[2024-10-01T18:20:04.150Z] [----------] 72 tests from test_bcast_alg
[2024-10-01T18:20:04.150Z] [ RUN      ] test_bcast_alg.0

it will look like this:

[----------] 24 tests from test_bcast_alg
[ RUN      ] test_bcast_alg.0UCC_CL_HIER_TUNEbcast2step0infinfUCC_CLSall815

for every test

[janjust]

The test in on itself works as intended, the question is is there a fallback in the infra, hece why we're not seeing a failure.
@B-a-S , Where can we access these machines? We need to verify if we're actually running MCAST enabled test suite.

[samnordmann]

The test in on itself works as intended, the question is is there a fallback in the infra, hece why we're not seeing a failure. @B-a-S , Where can we access these machines? We need to verify if we're actually running MCAST enabled test suite.

I thought the conclusion was that the CI on this PR should fail and reveal the bug fixed in #1022. Is there any update on this? Do we want to merge this PR regardless?

[janjust]

Hey Sam, you are correct this PR should in fact fail, and it gives a false negative. The false negative seems to be that we're not running MCAST and instead are falling back or just ignoring the TL_MLX5. I need to dig further and haven't had a chance to look at it. But outside of the CI the test indeed fails.
I think we should merge this test, however. Just keep in mind about the general CI of any MCAST tests. I'll follow up on this.
But as is, I think it's ok to merge.

[janjust]

@MamziB rebase please