Carry option_skip, and make test item ids stable

Project.toml

@@ -31,7 +31,7 @@ TestItemRunner = "f8b46487-2199-4994-9208-9a1283c18c0a"
 Test = "8dfed614-e22c-5e08-85e1-65c5234f0b40"
 
 [compat]
-TestItemDetection = "1.1"
+TestItemDetection = "1.2"
 JuliaSyntax = "0.4, 1"
 PrecompileTools = "1.2"
 SHA = "<0.0.1, 0.7, 1"

src/layer_testitems.jl

@@ -57,6 +57,36 @@ Salsa.@derived function derived_testitems_selected(rt, uri)
     return path_selected(parse_path_filter!(discard, toml_content), relpath)
 end
 
+"""
+    testitem_relative_path(package_uri, uri) -> String
+
+The path of `uri` relative to its package root, with `/` separators. This is the
+first half of a test item id, so it must be stable across machines: relative so a
+dev checkout and a CI runner agree, `/`-separated so Windows and Linux do. Falls
+back to the full URI when there is no filesystem path to work with.
+"""
+function testitem_relative_path(package_uri::Union{URI,Nothing}, uri::URI)
+    if package_uri !== nothing
+        package_path = uri2filepath(package_uri)
+        file_path = uri2filepath(uri)
+
+        if package_path !== nothing && file_path !== nothing
+            relpath = config_relative_path(package_path, file_path)
+            relpath === nothing || return relpath
+        end
+    end
+
+    return string(uri)
+end
+
+function _label_counts(labels)
+    counts = Dict{String,Int}()
+    for label in labels
+        counts[label] = get(counts, label, 0) + 1
+    end
+    return counts
+end
+
 Salsa.@derived function derived_testitems(rt, uri)
     @debug "derived_testitems" uri=uri
 
@@ -118,17 +148,71 @@ Salsa.@derived function derived_testitems(rt, uri)
         )
     end
 
+    all_testerrors = TestErrorDetail[
+        TestErrorDetail(
+            uri,
+            "$uri:error$i",
+            string(te.name),
+            te.message,
+            te.range
+        ) for (i,te) in enumerate(testerrors)
+    ]
+
+    # Ids are `<path relative to the package>::<label>`, so inserting a test item
+    # above another one no longer renumbers it. A label used more than once in one
+    # file is a definition error, but the run must still degrade rather than break,
+    # so *every* occurrence gets a `#N` suffix — that keeps ids unique, keeps each
+    # item individually addressable, and makes the error state visible in the id.
+    relpath = testitem_relative_path(package_uri, uri)
+
+    item_labels = String[ti.name for ti in testitems]
+    item_counts = _label_counts(item_labels)
+    seen_items = Dict{String,Int}()
+    item_ids = Vector{String}(undef, length(testitems))
+
+    for (i, label) in enumerate(item_labels)
+        if item_counts[label] > 1
+            occurrence = seen_items[label] = get(seen_items, label, 0) + 1
+            item_ids[i] = "$relpath::$label#$occurrence"
+
+            push!(all_testerrors, TestErrorDetail(
+                uri,
+                "$uri:error$(length(all_testerrors) + 1)",
+                label,
+                "The test item name \"$label\" is used more than once in this file. Test item names must be unique within a file.",
+                testitems[i].range
+            ))
+        else
+            item_ids[i] = "$relpath::$label"
+        end
+    end
+
+    setup_counts = _label_counts(String[string(ts.name) for ts in testsetups])
+
+    for ts in testsetups
+        if setup_counts[string(ts.name)] > 1
+            push!(all_testerrors, TestErrorDetail(
+                uri,
+                "$uri:error$(length(all_testerrors) + 1)",
+                string(ts.name),
+                "The test setup name `$(ts.name)` is used more than once in this file. Test setup names must be unique within a file.",
+                ts.range
+            ))
+        end
+    end
+
     return TestDetails(
         [TestItemDetail(
             uri,
-            "$uri:$i",
+            item_ids[i],
             ti.name,
             text_file.content.content[ti.code_range],
             ti.range,
             ti.code_range,
             ti.option_default_imports,
             ti.option_tags,
-            ti.option_setup
+            ti.option_setup,
+            ti.option_skip isa Bool ? ti.option_skip : text_file.content.content[ti.option_skip]
             ) for (i,ti) in enumerate(testitems)],
         [TestSetupDetail(
             uri,
@@ -138,13 +222,7 @@ Salsa.@derived function derived_testitems(rt, uri)
             i.range,
             i.code_range
             ) for i in testsetups],
-        [TestErrorDetail(
-            uri,
-            "$uri:error$i",
-            string(te.name),
-            te.message,
-            te.range
-            ) for (i,te) in enumerate(testerrors)]
+        all_testerrors
     )
 end
 

src/types.jl

@@ -19,6 +19,8 @@ Details of a test item.
 - `option_default_imports`::Bool
 - option_tags::Vector{Symbol}
 - option_setup::Vector{Symbol}
+- `option_skip`::Union{Bool,String} — a literal `true`/`false`, or the source text of an
+  expression that the test process evaluates just before the test item would run.
 """
 struct TestItemDetail
     uri::URI
@@ -30,8 +32,9 @@ struct TestItemDetail
     option_default_imports::Bool
     option_tags::Vector{Symbol}
     option_setup::Vector{Symbol}
+    option_skip::Union{Bool,String}
 end
-_key(x::TestItemDetail) = (x.uri, x.id, x.name, x.code, _range_key(x.range), _range_key(x.code_range), x.option_default_imports, x.option_tags, x.option_setup)
+_key(x::TestItemDetail) = (x.uri, x.id, x.name, x.code, _range_key(x.range), _range_key(x.code_range), x.option_default_imports, x.option_tags, x.option_setup, x.option_skip)
 Base.:(==)(a::TestItemDetail, b::TestItemDetail) = _key(a) == _key(b)
 Base.isequal(a::TestItemDetail, b::TestItemDetail) = isequal(_key(a), _key(b))
 Base.hash(x::TestItemDetail, h::UInt) = hash(_key(x), hash(TestItemDetail, h))

test/test_testitems.jl

@@ -658,8 +658,8 @@ end
     @test !isequal(te_a, te_b)
     @test hash(te_a) != hash(te_b)
 
-    ti_a = TestItemDetail(u, "id", "n", "code", 24:23, 24:23, true, Symbol[], Symbol[])
-    ti_b = TestItemDetail(u, "id", "n", "code", 23:22, 23:22, true, Symbol[], Symbol[])
+    ti_a = TestItemDetail(u, "id", "n", "code", 24:23, 24:23, true, Symbol[], Symbol[], false)
+    ti_b = TestItemDetail(u, "id", "n", "code", 23:22, 23:22, true, Symbol[], Symbol[], false)
     @test ti_a != ti_b
     @test !isequal(ti_a, ti_b)
     @test hash(ti_a) != hash(ti_b)
@@ -672,10 +672,99 @@ end
 
     # Identical values still compare equal (backdating must still work).
     @test te_b == TestErrorDetail(u, "id", "n", "msg", 23:22)
-    @test isequal(ti_b, TestItemDetail(u, "id", "n", "code", 23:22, 23:22, true, Symbol[], Symbol[]))
+    @test isequal(ti_b, TestItemDetail(u, "id", "n", "code", 23:22, 23:22, true, Symbol[], Symbol[], false))
     @test hash(ts_b) == hash(TestSetupDetail(u, :n, :k, "code", 23:22, 23:22))
 end
 
+@testsnippet TestItemPackage begin
+    using JuliaWorkspaces: JuliaWorkspace, add_file!, TextFile, SourceText, get_test_items
+    using JuliaWorkspaces.URIs2: @uri_str, URI
+
+    # A minimal in-memory package, so that test items in `test/bar.jl` resolve to a
+    # package root and therefore get an id relative to it.
+    function workspace_with(content)
+        jw = JuliaWorkspace()
+
+        add_file!(jw, TextFile(uri"file:///home/foo/Project.toml", SourceText("name = \"Foo\"\nuuid = \"12345678-1234-1234-1234-123456789012\"\nversion = \"0.1.0\"\n", "toml")))
+        add_file!(jw, TextFile(uri"file:///home/foo/src/Foo.jl", SourceText("module Foo\nend\n", "julia")))
+        add_file!(jw, TextFile(uri"file:///home/foo/test/bar.jl", SourceText(content, "julia")))
+
+        return jw, uri"file:///home/foo/test/bar.jl"
+    end
+end
+
+@testitem "skip defaults to false" setup=[TestItemPackage] begin
+    jw, uri = workspace_with("""@testitem "foo" begin end""")
+
+    test_results = get_test_items(jw, uri)
+
+    @test length(test_results.testitems) == 1
+    @test test_results.testitems[1].option_skip === false
+end
+
+@testitem "skip literal is carried through" setup=[TestItemPackage] begin
+    jw, uri = workspace_with("""@testitem "foo" skip=true begin end\n@testitem "bar" skip=false begin end""")
+
+    test_results = get_test_items(jw, uri)
+
+    @test length(test_results.testerrors) == 0
+    @test test_results.testitems[1].option_skip === true
+    @test test_results.testitems[2].option_skip === false
+end
+
+@testitem "skip expression is carried through as source text" setup=[TestItemPackage] begin
+    jw, uri = workspace_with("""@testitem "foo" skip=(VERSION < v"1.11") begin end""")
+
+    test_results = get_test_items(jw, uri)
+
+    @test length(test_results.testerrors) == 0
+    # Parentheses are trivia to JuliaSyntax, so only the expression itself is sliced.
+    @test test_results.testitems[1].option_skip == "VERSION < v\"1.11\""
+end
+
+@testitem "test item ids are package relative and label based" setup=[TestItemPackage] begin
+    jw, uri = workspace_with("""@testitem "foo" begin end\n@testitem "bar" begin end""")
+
+    test_results = get_test_items(jw, uri)
+
+    @test [ti.id for ti in test_results.testitems] == ["test/bar.jl::foo", "test/bar.jl::bar"]
+end
+
+@testitem "test item ids are invariant under inserting an item above" setup=[TestItemPackage] begin
+    jw1, uri = workspace_with("""@testitem "foo" begin end""")
+    jw2, _ = workspace_with("""@testitem "inserted" begin end\n@testitem "foo" begin end""")
+
+    id1 = only(get_test_items(jw1, uri).testitems).id
+    id2 = get_test_items(jw2, uri).testitems[2].id
+
+    @test id1 == id2 == "test/bar.jl::foo"
+end
+
+@testitem "duplicate test item labels get numbered ids and a definition error" setup=[TestItemPackage] begin
+    jw, uri = workspace_with("""@testitem "foo" begin end\n@testitem "foo" begin end\n@testitem "bar" begin end""")
+
+    test_results = get_test_items(jw, uri)
+
+    # Every occurrence is suffixed, not just the second one, so the error state is
+    # visible in the id itself and each item stays individually addressable.
+    @test [ti.id for ti in test_results.testitems] == ["test/bar.jl::foo#1", "test/bar.jl::foo#2", "test/bar.jl::bar"]
+
+    @test length(test_results.testerrors) == 2
+    @test all(te -> te.name == "foo", test_results.testerrors)
+    @test all(te -> occursin("used more than once", te.message), test_results.testerrors)
+    @test allunique(te.id for te in test_results.testerrors)
+end
+
+@testitem "duplicate test setup names produce a definition error" setup=[TestItemPackage] begin
+    jw, uri = workspace_with("""@testmodule Foo begin end\n@testsnippet Foo begin end""")
+
+    test_results = get_test_items(jw, uri)
+
+    @test length(test_results.testsetups) == 2
+    @test length(test_results.testerrors) == 2
+    @test all(te -> te.name == "Foo", test_results.testerrors)
+end
+
 @testitem "get_test_items on untitled (non-file) URI" begin
     using JuliaWorkspaces: JuliaWorkspace, add_file!, TextFile, SourceText, get_test_items
     using JuliaWorkspaces.URIs2: @uri_str