use ScopedValues for TestSets #53462

simonbyrne · 2024-02-25T07:26:45Z

Another attempt at #51012

simonbyrne · 2024-02-28T05:13:39Z

@vchuravy @Keno I'm getting some weird failures on this branch originating from this line:

Line 1162 in c379db7

@assert current_mapping == val

The simplest reproducer I have is:

using Test
@testset begin
    @testset for r in Int[1]
    end
end

simonbyrne · 2024-02-29T06:09:40Z

Blocked by #53521

nickrobinson251 · 2024-09-23T19:26:29Z

Would love to see this merged!

@simonbyrne do you have any time to work on this? I'd be happy to rebase and help get it over the line. I think rebasing, and then deciding what to do with the commented-out scope tests might be all that's needed here?

stdlib/Test/src/Test.jl

giordano · 2024-09-23T19:54:05Z

I initially thought I managed to badly botch the rebase, but I believe this is now failing to build because after the rebase #55452 was merged in and that PR removed using .ScopedValues inside Base, so imports need to change a little bit. I'll leave this exercise to someone else 😁

simonbyrne · 2024-09-23T19:55:17Z

Sorry, it still needs some work. The handling of errors is wasn't quite correct, and the resetting of rng state is kind of confusing (arguably should be handled differently)

base/scopedvalues.jl

simonbyrne · 2024-10-09T04:34:02Z

Currently blocked by #56062

d-netto · 2025-01-30T15:13:25Z

FYI, #56062 was recently fixed.

simonbyrne · 2025-02-26T05:32:55Z

Okay, I think this is now passing. Any thoughts?

simonbyrne · 2025-03-03T00:18:47Z

@nanosoldier runtests()

giordano

Even though I recently touched the Test stdlib I'm not particularly familiar with its code, I don't understand some of the design choices. That said, this looks overall good to me (as much as that mean, not knowing the code), looks like a decent simplification. I'm going to ~~trust~~ hope existing tests already cover corner cases of nested testsets that we should make sure keep working.

stdlib/Test/test/runtests.jl

stdlib/Test/src/Test.jl

giordano · 2025-03-02T23:59:25Z

test/scopedvalues.jl

-    @test sprint(show, Core.current_scope()) == "nothing"
+    @test sprint(show, ScopedValue{Int}()) == "$ScopedValue{$Int}(undefined)"
+    @test sprint(show, sval) == "$ScopedValue{$Int}(1)"
+    # @test sprint(show, Core.current_scope()) == "nothing"


Why was this test commented out? Can we move it at the top-level, outside of any testset, to keep the test, or that's still within an implicit scope?

Depending on how the runtests is called, it might be inside a scope. The only way to guarantee it is to run it in a separate process.

giordano · 2025-03-02T23:59:57Z

test/scopedvalues.jl

+        @test sprint(show, sval) == "$ScopedValue{$Int}(2)"
        objid = sprint(show, Base.objectid(sval))
-        @test sprint(show, Core.current_scope()) == "Base.ScopedValues.Scope(Base.ScopedValues.ScopedValue{$Int}@$objid => 2)"
+        # @test sprint(show, Core.current_scope()) == "Base.ScopedValues.Scope(Base.ScopedValues.ScopedValue{$Int}@$objid => 2)"


Also this: why was it removed?

Same, I can't see a way to make this work.

giordano · 2025-03-03T00:35:45Z

stdlib/Test/src/Test.jl

-"""
-    push_testset(ts::AbstractTestSet)
-
-Adds the test set to the `task_local_storage`.
-"""
-function push_testset(ts::AbstractTestSet)
-    testsets = get(task_local_storage(), :__BASETESTNEXT__, AbstractTestSet[])
-    push!(testsets, ts)
-    setindex!(task_local_storage(), testsets, :__BASETESTNEXT__)
-end


Apparently some (testing) packages were using this: https://juliahub.com/ui/Search?type=symbols&q=push_testset&u=use. Presumably it's ok to ask them to adapt their use of internal functions? These aren't documented.

Yes, unfortunately I don't see how we could keep that working

How would one update a codebase that is using push_testset? is it as simple as changing from

push_testset(ts) try ... finally pop_testset() end

to

@with_testset ts begin ... end

?

(p.s. Very pleased to see this improvement!)

Yes, that should work I think.

giordano · 2025-03-03T00:37:07Z

stdlib/Test/src/Test.jl

-                get_testset_depth() > 1 ? rethrow() : failfast_print()
+                get_testset_depth() > 0 ? rethrow() : failfast_print()


I can't immediately tell the motivation for this change, could you please elaborate?

Previously the check used to be inside the scope, now it is outside

nanosoldier · 2025-03-05T18:01:07Z

The package evaluation job you requested has completed - possible new issues were detected.
The full report is available.

Report summary

❗ Packages that crashed

3 packages crashed only on the current version.

An internal error was encountered: 1 packages
A segmentation fault happened: 2 packages

14 packages crashed on the previous version too.

✖ Packages that failed

181 packages failed only on the current version.

Package fails to precompile: 19 packages
Package has test failures: 3 packages
Package tests unexpectedly errored: 133 packages
Test duration exceeded the time limit: 26 packages

1132 packages failed on the previous version too.

✔ Packages that passed tests

22 packages passed tests only on the current version.

Other: 22 packages

5158 packages passed tests on the previous version too.

~ Packages that at least loaded

18 packages successfully loaded only on the current version.

Other: 18 packages

2933 packages successfully loaded on the previous version too.

➖ Packages that were skipped altogether

897 packages were skipped on the previous version too.

Test has `Test.record_passes()` which gets `OncePerProcess` initialized to the value of `ENV["JULIA_TEST_RECORD_PASSES"]`. However, in another palce the test suite once to override this value for its scope, and does this by setting the environment variable, relying on OncePerProcess picking this up. However, this is not at all semantically guaranteed and of course fails if anybody happened to have a testset before (which is possible, because this is used by Base.runtests). It would be much better if we could use a ScopedValue for this like the other `Test` setting state (after #53462). However, there currently isn't any way to once-initialize the ScopedValue default. This PR aims to fix that by letting the scoped value default be an evaluated callback. The original `ScopedValue` struct is renamed `LazyScopedValue`, but the `ScopedValue` name is retained with the same API as an alias to a `LazyScopedValue` that just uses `Returns(default)` as its initializer. It might be a bit weird that the default could return a different value at every time, but I think a reasonable way to think about this is that the default chains to some dynamic state which is expressed in this callback.

Keno · 2025-08-24T05:23:43Z

Sigh, I don't understand the test failures here. I can't reproduce them locally and worse, #59372 which includes these commits doesn't show them either - and yet - the tests failed on all platforms on CI.

Keno · 2025-08-24T06:10:28Z

I'm gonna go with testsets are not thread safe, and before this change, they did not propagate into @threads, but now they do.

Keno · 2025-08-24T07:02:23Z

  TaskFailedException
      nested task error: ConcurrencyViolationError("Vector has invalid state. Don't modify internal fields incorrectly, or resize without correct locks")
      Stacktrace:
        [1] _growend_internal!(a::Vector{Any}, delta::Int64, len::Int64)
          @ Base ./array.jl:1141
        [2] _growend!
          @ ./array.jl:1181 [inlined]
        [3] push!
          @ ./array.jl:1320 [inlined]
        [4] record

The retry confirmed that theory

In preparation of #53462, ensure that attempting to record test results to a test set from multiple threads does not cause corruption. Note that other part of `Test` remain non-threadsafe.

Keno · 2025-08-25T04:33:35Z

I have rebased on top of #59374, in the hope that that will fix CI and the expectation that that'll get merged first.

In preparation of #53462, ensure that attempting to record test results to a test set from multiple threads does not cause corruption. Note that other part of `Test` remain non-threadsafe.

Test has `Test.record_passes()` which gets `OncePerProcess` initialized to the value of `ENV["JULIA_TEST_RECORD_PASSES"]`. However, in another palce the test suite once to override this value for its scope, and does this by setting the environment variable, relying on OncePerProcess picking this up. However, this is not at all semantically guaranteed and of course fails if anybody happened to have a testset before (which is possible, because this is used by Base.runtests). It would be much better if we could use a ScopedValue for this like the other `Test` setting state (after #53462). However, there currently isn't any way to once-initialize the ScopedValue default. This PR aims to fix that by letting the scoped value default be an evaluated callback.

In preparation of #53462, ensure that attempting to record test results to a test set from multiple threads does not cause corruption. Note that other part of `Test` remain non-threadsafe.

simonbyrne · 2025-08-26T15:46:01Z

Thanks Keno for seeing this through!

NHDaly · 2025-08-28T20:56:04Z

Amazing, I'm glad to see this! 💪 This is a really nice cleanup, and a perfect showcase for ScopedValues. 👏

Test has `Test.record_passes()` which gets `OncePerProcess` initialized to the value of `ENV["JULIA_TEST_RECORD_PASSES"]`. However, in another palce the test suite once to override this value for its scope, and does this by setting the environment variable, relying on OncePerProcess picking this up. However, this is not at all semantically guaranteed and of course fails if anybody happened to have a testset before (which is possible, because this is used by Base.runtests). It would be much better if we could use a ScopedValue for this like the other `Test` setting state (after #53462). However, there currently isn't any way to once-initialize the ScopedValue default. This PR aims to fix that by letting the scoped value default be an evaluated callback.

simonbyrne force-pushed the sb/test-scope branch 6 times, most recently from 1c67443 to b1fec32 Compare February 27, 2024 05:38

simonbyrne force-pushed the sb/test-scope branch 2 times, most recently from 73b39d1 to 0cd361c Compare March 9, 2024 01:00

simonbyrne force-pushed the sb/test-scope branch from 0cd361c to 7ef5878 Compare March 17, 2024 16:41

simonbyrne force-pushed the sb/test-scope branch from 7ef5878 to 7849324 Compare May 25, 2024 17:42

giordano added testsystem The unit testing framework and Test stdlib stdlib Julia's standard library labels Sep 23, 2024

giordano reviewed Sep 23, 2024

View reviewed changes

stdlib/Test/src/Test.jl Outdated Show resolved Hide resolved

simonbyrne marked this pull request as ready for review October 5, 2024 03:51

IanButterworth mentioned this pull request Oct 7, 2024

Fix no-arg ScopedValues.@with within a scope #56019

Merged

IanButterworth reviewed Oct 7, 2024

View reviewed changes

base/scopedvalues.jl Outdated Show resolved Hide resolved

simonbyrne force-pushed the sb/test-scope branch from 1a89ef6 to 449a7ab Compare January 31, 2025 17:28

simonbyrne requested review from giordano and vchuravy February 26, 2025 05:33

giordano reviewed Mar 3, 2025

View reviewed changes

Keno mentioned this pull request Aug 24, 2025

Test: Make test recording thread safe #59374

Merged

Keno removed the merge me PR is reviewed. Merge when all tests are passing label Aug 25, 2025

Keno pushed a commit that referenced this pull request Aug 25, 2025

Rebase #53462

23cb5e9

Keno force-pushed the sb/test-scope branch from 2ab5e05 to 0d5a8b1 Compare August 25, 2025 04:33

Keno added merge me PR is reviewed. Merge when all tests are passing and removed merge me PR is reviewed. Merge when all tests are passing labels Aug 25, 2025

simonbyrne and others added 6 commits August 25, 2025 16:32

Rebase #53462

564e1d5

FAIL_FAST to OncePerProcess

b0b4cf9

rm unrelated ScopedValue changes

e3c4c64

Add NEWS

e6b0f73

Also move over TESTSET_PRINT_ENABLE

888b9aa

Turns out the scopedvalues test changes are required

e503185

Keno force-pushed the sb/test-scope branch from 0d5a8b1 to e503185 Compare August 25, 2025 16:32

Keno merged commit bb36851 into master Aug 26, 2025
7 checks passed

Keno deleted the sb/test-scope branch August 26, 2025 15:06

icweaver mentioned this pull request Sep 10, 2025

Make compatible with Julia v1.13 julia-vscode/TestItemRunner.jl#114

Open

		get_testset_depth() > 1 ? rethrow() : failfast_print()
		get_testset_depth() > 0 ? rethrow() : failfast_print()

Uh oh!

use ScopedValues for TestSets #53462

use ScopedValues for TestSets #53462

Uh oh!

Conversation

simonbyrne commented Feb 25, 2024

Uh oh!

simonbyrne commented Feb 28, 2024

Uh oh!

simonbyrne commented Feb 29, 2024

Uh oh!

nickrobinson251 commented Sep 23, 2024

Uh oh!

Uh oh!

giordano commented Sep 23, 2024

Uh oh!

simonbyrne commented Sep 23, 2024

Uh oh!

Uh oh!

simonbyrne commented Oct 9, 2024

Uh oh!

d-netto commented Jan 30, 2025

Uh oh!

simonbyrne commented Feb 26, 2025

Uh oh!

simonbyrne commented Mar 3, 2025

Uh oh!

giordano left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nanosoldier commented Mar 5, 2025

❗ Packages that crashed

✖ Packages that failed

✔ Packages that passed tests

~ Packages that at least loaded

➖ Packages that were skipped altogether

Uh oh!

Keno commented Aug 24, 2025

Uh oh!

Keno commented Aug 24, 2025

Uh oh!

Keno commented Aug 24, 2025

Uh oh!

Keno commented Aug 25, 2025

Uh oh!

Uh oh!

simonbyrne commented Aug 26, 2025

Uh oh!

NHDaly commented Aug 28, 2025

Uh oh!

Uh oh!