fix(test): Remove \c from echo string.

[jswxstw]

Fixes #13565

Motivation

echo in CentOS's built-in sh does not support the escape character \c, causing make test to fail.

CentOS 8.5

# uname -a
Linux VM-0-17-centos 4.18.0-348.7.1.el8_5.x86_64 #1 SMP Wed Dec 22 13:25:12 UTC 2021 x86_64 x86_64 x86_64 GNU/Linux

# sh -c 'echo "hello\c"'
hello\c

Modifications

Remove \c from echo string.

Verification

make test in different os.

[jswxstw]

/retest

[Joibel]

This PR is for "some os"es, but you don't specify which one(s). Could you be much more specific in your PR description please?

[Joibel]

This is a windows specific value for expected.

Why can't you leave the tests as they are but use a similar tactic for all of these tests instead of changing the inputs on all platforms?

[jswxstw]

It is difficult to determine whether the echo command used by the current OS supports \c.
I also considered the following two options:

1. using echo -e to explicitly enable escape characters, problems:
   • pwsh do not support -e
   • sh of old versions do not support -e
2. using echo -n instead of \c, problems:
   • pwsh do not support -n
   • fix: replace echo -n with echo ...\c in test #11576: -n also does not work for some other OS

So in the end I think it's better to just remove \c and keep the newline.

[Joibel]

My problem with changing this is that both of these tests it that the original author specifically added a \c to the code.

This implies they wanted to test this case, and changing the test case for all the platforms feels like the wrong approach.

If it doesn't work for you in your native environment can you run inside the devcontainer where it has a shell that works with this?

[jswxstw]

My problem with changing this is that both of these tests it that the original author specifically added a \c to the code.

If it just want to test whether \c is effective here, according to the existing logic, except for Windows, other OS are expected to be effective. However, on CentOS it does not work as expected. Is this a problem of CentOS which means argo cannot run on it, or is there a problem with this unit test itself?

If it doesn't work for you in your native environment can you run inside the devcontainer where it has a shell that works with this?

I just wish make test works for all platforms to simplify development.

[agilgur5]

The original PR with \c replaced the -n due to test failures: #11576

The original -n is from #11368. I don't think it was necessary in that PR's context

[Joibel]

So my take is that #11368 is about ensuring that all of stdout is captured, and the tests are trying to prove that.

Stdout is buffered and connected to a terminal, so will only be flushed on a new line. Reference:
https://www.pixelbeat.org/programming/stdio_buffering/

So by putting the new line in to the echo we'll end up with a test which no longer tests that buffered output which hasn't been flushed is captured by the system.

The closer introduced in #11368 is there to force flush stdout, and these tests are there to create buffered unflushed output and prove we capture it.

[agilgur5]

Thanks for the analysis Alan! We should definitely add a comment to mention that the newline is intentionally there for testing buffering / flushing 😅

In that case, I would suggest detecting CentOS and having a different behavior for it, since this seems to affect variants / derivatives of CentOS per #13565

[jswxstw]

I don't get it. #11368 add cmd.Wait() to ensure the command completed and all output was written to stdout, why does it matter with '\n'？
In my opinion, this test just checks whether hello in stdout is successfully captured. Why don't we just check if the output is consistent with hello\n instead of investigating the OS suitability of -n and \c, which have been proven to be applicable to different OSes.

[isubasinghe]

So I went down this rabbit hole today, is it not bad that the delay we give to copy IO before erroring out is just a hard coded 100ms?

[jswxstw]

This PR is for "some os"es, but you don't specify which one(s). Could you be much more specific in your PR description please?

Enterprise customized version compatible with CentOS:

# uname -a
Linux 5.4.241-1-tlinux4-0017.9 #1 SMP Sun Apr 7 15:58:02 CST 2024 x86_64 x86_64 x86_64 GNU/Linux

# sh -c 'echo "hello\c"'
hello\c

CentOS 8.5:

# uname -a
Linux VM-0-17-centos 4.18.0-348.7.1.el8_5.x86_64 #1 SMP Wed Dec 22 13:25:12 UTC 2021 x86_64 x86_64 x86_64 GNU/Linux

# sh -c 'echo "hello\c"'
hello\c

[Joibel]

Using echo -n in a centos docker image works for me.

Can we determine which of the mechanisms (-n vs \c) is the correct one and dynamically execute the correct command?