Tweak to the -s argument and the log message to match

[aweeraman]

Small tweak to the recent fix for the b_sync test.

[aweeraman]

@krader1961 I attempted to validate this fix in Debian and the test appears to be still failing. I issued a separate PR on test failures due to missing home directory, and I'm just wondering whether this and b_hist.exp test failures that I'm experiencing could also be related to that. I'll keep digging a little further.

Build log: https://salsa.debian.org/debian/ksh/-/jobs/396918

SHELL='/builds/debian/ksh/debian/output/ksh-2020.0.0/obj-x86_64-linux-gnu/src/cmd/ksh93/ksh' LIBSAMPLE_PATH='/builds/debian/ksh/debian/output/ksh-2020.0.0/obj-x86_64-linux-gnu/src/lib/libdll/libsample.so' TEST_ROOT='/builds/debian/ksh/debian/output/ksh-2020.0.0/src/cmd/ksh93/tests' LD_LIBRARY_PATH='/builds/debian/ksh/debian/output/ksh-2020.0.0/obj-x86_64-linux-gnu/src/lib/libast:/builds/debian/ksh/debian/output/ksh-2020.0.0/obj-x86_64-linux-gnu/src/lib/libcmd:/builds/debian/ksh/debian/output/ksh-2020.0.0/obj-x86_64-linux-gnu/src/lib/libdll' SRC_ROOT='/builds/debian/ksh/debian/output/ksh-2020.0.0' SHCOMP='/builds/debian/ksh/debian/output/ksh-2020.0.0/obj-x86_64-linux-gnu/src/cmd/ksh93/shcomp' LANG='en_US.UTF-8' /builds/debian/ksh/debian/output/ksh-2020.0.0/obj-x86_64-linux-gnu/src/cmd/ksh93/ksh /builds/debian/ksh/debian/output/ksh-2020.0.0/src/cmd/ksh93/tests/util/run_test.sh shcomp b_sync
stdout:
run_test[121]: TEST_DIR=/tmp/ksh.b_sync.r4t9Lo
run_test[162]: ITERS_PER_10MS=32000
stderr:
b_sync[28]: sync -s999
expect: |sync: fsync(999) failed|
actual: ||
b_sync[-1]: error_count = 1

[krader1961]

If the removal of the space between the short option and its value changes the behavior that means getopt_long(), or its use by b_sync(), is seriously broken. That is unlikely given that it works correctly on quite a few platforms. What is more likely is there is something odd about your test environment. Perhaps your virtual-machine or other sandbox behavior that makes fsync(999) seem to succeed when it should fail.

P.S., the log_error() portion of your change is correct; albeit solely to avoid confusion for a human reader of the test.

[aweeraman]

I agree the space between the -s and 999 doesn't have any impact, just thought I'd make it consistent with the prior test. I only wanted to update the log message as it confused me initially while reviewing the output.

[krader1961]

It is actually a net positive that tests vary such seemingly trivial details as the spacing between options and their value. Attempting 100% consistency on such points, that are otherwise irrelevant to the real purpose of the test, is counter-productive. Such inconsistencies help to find bugs and help ensure readers of the code do not make unwarranted assumptions.

[aweeraman]

It has been reverted...

[krader1961]

You'll probably need to use strace and lsof to understand what's going on.

[aweeraman]

Yeah, the trouble is that I'm not able to recreate this locally. I agree strace would help understand what's going on. Will poke around a bit more, learning ksh and how you guys have laid things out along the way. Impressive.

[krader1961]

Honestly, my recommendation is to just blacklist this test. See

ast/src/cmd/ksh93/tests/util/run_test.sh

Lines 20 to 40 in 11983a7

	tests_to_skip=(
	'cygwin b_jobs.exp'
	'cygwin b_time.exp'
	'cygwin b_times.exp'
	'cygwin b_ulimit.sh'
	'cygwin b_uname.sh' # too many trivial diffs in uname output
	'cygwin coprocess.sh'
	'cygwin signal.sh'
	'cygwin sh_match.sh' # too slow, times out
	'openbsd b_times.exp'
	'sunos vi.exp'
	'sunos directoryfd.sh'
	'wsl b_time.exp'
	'wsl b_times.exp'
	'wsl sh_match.sh' # too slow, times out
	# Tests to be skipped because they are known to be broken when compiled by `shcomp`.
	# TODO: Fix these tests or the shcomp code.
	'shcomp io.sh'
	'shcomp b_set.sh'
	'shcomp treemove.sh'
	)

And

ast/src/cmd/ksh93/tests/util/run_test.sh

Lines 109 to 120 in 11983a7

	# If the test should be skipped do so.
	readonly test_name=$1
	[[ $shcomp == skip && test_name == *.exp ]] && exit $MESON_SKIPPED_TEST
	for test in "${tests_to_skip[@]}"
	do
	system=${test% *}
	test_to_skip=${test#* }
	[[ $shcomp == true && $system == shcomp && $test_to_skip == $test_name ]] && \
	exit $MESON_SKIPPED_TEST
	[[ $system == $MESON_SYSTEM && $test_to_skip == $test_name ]] && \
	exit $MESON_SKIPPED_TEST
	done

Although I would predicate skipping it on running inside a Salsa CI context, rather than the platform being Debian, if that is possible.

[krader1961]

@aweeraman I've merged this. As I said earlier you might want to consider just blacklisting the b_sync.sh unit test when running on Salsa CI if you're not able to figure out why that CI environment affects this test.

[aweeraman]

Will do, thanks.