Distributed test suite: if Threads.nthreads() > 1, skip certain tests

[DilumAluthge]

1. If Threads.nthreads() == 1, then we will run all tests.
2. If Threads.nthreads() > 1:
   • If ENV["JULIA_TEST_INCLUDE_THREAD_UNSAFE"] == "true", then we will run all tests.
   • Else, we will skip a few tests that are known to non-deterministically fail when there are multiple threads.

[tkf]

Yes, I think it's nice to mark thread-incompatible tests like this.

[DilumAluthge]

@tkf Should we rename some of the other functions to reflect the new name of the environment variable?

[tkf]

Yeah, maybe that's a good idea since it'd make it easy to move this to testhelper later if we want to use it elsewhere.

[DilumAluthge]

Any suggestions on a good naming scheme?

[tkf]

I think reflecting the environment variable, like you initially did, is a good approach. So run_distributed_multithreaded -> skip_if_multithreaded.

[DilumAluthge]

What about the naming for skipping tests?

Right now we have:

if run_distributed_multithreaded()
    @test f()
end

Maybe something like:

if !skip_this_test_if_multithreaded()
    @test f()
end

I don't love the double negative though.....

[tkf]

I like "skip" here since it is similar to @test_skip and so I think it outweighs the downside of double-negative. That said, I think using something like include_thread_unsafe is good too.

[DilumAluthge]

Marking this as a draft until I finish the renaming.

[DilumAluthge]

I'd like to avoid using double negatives, so I chose to go with include_thread_unsafe().

The environment variable is now named JULIA_TEST_INCLUDE_THREAD_UNSAFE.

And the function is now named include_thread_unsafe().

[tkf]

LGTM!

[DilumAluthge]

I'm going to wait for CI to finish, and then I'll check the CI logs to make sure the warning correctly shows up in the logs of the multi-threaded CI jobs.

[DilumAluthge]

The Buildkite logs look correct.

This PR is good to merge once CI is green.

[KristofferC]

If Distributed needs to be tested both threaded and unthreaded it should just do so, like:

julia/test/threads.jl

Lines 5 to 11 in d39b2c0

	let cmd = `$(Base.julia_cmd()) --depwarn=error --rr-detach --startup-file=no threads_exec.jl`
	for test_nthreads in (1, 2, 4, 4) # run once to try single-threaded mode, then try a couple times to trigger bad races
	new_env = copy(ENV)
	new_env["JULIA_NUM_THREADS"] = string(test_nthreads)
	run(pipeline(setenv(cmd, new_env), stdout = stdout, stderr = stderr))
	end
	end

It should not be an external "setting" so that to fully test Julia, one needs to run the full test suite multiple times with the different settings when 99%+ of the tests execute exactly the same no matter that setting.

[DilumAluthge]

This is incorrect. The entire Julia test suite needs to be run both singlethreaded and multithreaded, at least on a single platform. There are bugs that only show up in one case versus the other.

Just off the top of my head, there was a test for one of the invalid sysimage code paths that passed when singlethreaded but failed when multithreaded. More specifically, we were testing that there would not be a segfault when exercising a certain code path. However, when we ran the tests with multiple threads enabled, the test segfaulted. This indicated that there was a bug in this particular code path.

That test was not part of the Distributed test suite.

[DilumAluthge]

when 99%+ of the tests execute exactly the same no matter that setting.

This is just not true. There are all kinds of subtle bugs in Julia that are absent when threading is disabled and are present when threading is enabled. The only way to test for these bugs is to run the test suite both with and without threading.

As I said on Slack, in order to conserve CI resources, we only do this on linux64. On linux64, we run the test suite both with and without threading. On all other architectures and operating systems, we only run the test suite once.