Do all filtering of testitems at AST level (#179)

* WIP do all filtering at AST level

* Bump version

* Update tests to better reflect rai macro

Specifically `name` is required, `tags` is supported,
`code` must be passed as a keyword argument.

* fixup! WIP do all filtering at AST level

* Delete internal tests for unsupported usage

Author: nickrobinson251

Project.toml

@@ -1,6 +1,6 @@
 name = "ReTestItems"
 uuid = "817f1d60-ba6b-4fd5-9520-3cf149f6a823"
-version = "1.25.1"
+version = "1.26.0"
 
 [deps]
 Dates = "ade2ca70-3891-5945-98fb-dc099432e06a"

src/filtering.jl

@@ -19,13 +19,6 @@ function (f::TestItemFilter)(ti::TestItemMetadata)
     return f.shouldrun(ti)::Bool && _shouldrun(f.tags, ti) && _shouldrun(f.name, ti)
 end
 
-# This method is needed for filtering `TestItem`s that were created by
-# `___RAI_MACRO_NAME_DONT_USE` and so couldn't be filtered from the AST.
-# TODO: drop this method when we no longer support `___RAI_MACRO_NAME_DONT_USE`
-function (f::TestItemFilter)(ti::TestItem)
-    return f.shouldrun(ti)::Bool && _shouldrun(f.tags, ti) && _shouldrun(f.name, ti)
-end
-
 _shouldrun(name::AbstractString, ti) = name == ti.name
 _shouldrun(pattern::Regex, ti) = contains(ti.name, pattern)
 _shouldrun(tags::AbstractVector{Symbol}, ti) = issubset(tags, ti.tags)
@@ -57,17 +50,21 @@ end
 # having to maintain our own version of that code.
 function filter_testitem(f, expr)
     @assert expr.head == :macrocall
-    if expr.args[1] !== Symbol("@testitem")
+    macro_name = expr.args[1]
+    if macro_name === Symbol("@testitem")
+        # `@testitem` have have at least: macro_name, line_number, name, body
+        length(expr.args) < 4 && return expr
+        name = try_get_name(expr)
+        name === nothing && return expr
+        tags = try_get_tags(expr)
+        tags === nothing && return expr
+        ti = TestItemMetadata(name, tags)
+        return f(ti) ? expr : nothing
+    elseif macro_name === Symbol("@testsetup")
         return expr
+    elseif macro_name === ___RAI_MACRO_NAME_DONT_USE # TODO: drop this branch when we can
+        return __filter_rai(f, expr)
     end
-    # `@testitem` have have at least: macro_name, line_number, name, body
-    length(expr.args) < 4 && return expr
-    name = try_get_name(expr)
-    name === nothing && return expr
-    tags = try_get_tags(expr)
-    tags === nothing && return expr
-    ti = TestItemMetadata(name, tags)
-    return f(ti) ? expr : nothing
 end
 
 # Extract the name from a `@testitem`, return `nothing` if name is not of the expected type.
@@ -104,6 +101,34 @@ end
 # Macro used by RAI (corporate sponsor of this package)
 # TODO: drop support for this when RAI codebase is fully switched to ReTestItems.jl
 const ___RAI_MACRO_NAME_DONT_USE = Symbol("@test_rel")
+# This logic is very similar to `try_get_tags` but we want to keep the two separate
+# so that this code is easy to delete when we drop support for the RAI macro.
+function __filter_rai(f, expr)
+    @assert expr.head == :macrocall && expr.args[1] == ___RAI_MACRO_NAME_DONT_USE
+    name = nothing
+    tags = Symbol[]
+    for args in expr.args[2:end]
+        if args isa Expr && args.head == :(=) && args.args[1] == :name && args.args[2] isa String
+            name = args.args[2]
+        elseif args isa Expr && args.head == :(=) && args.args[1] == :tags
+            tags_arg = args.args[2]
+            if tags_arg isa Expr && tags_arg.head == :vect
+                for tag in tags_arg.args
+                    if tag isa QuoteNode && tag.value isa Symbol
+                        push!(tags, tag.value)
+                    else
+                        return expr
+                    end
+                end
+            else
+                return expr
+            end
+        end
+    end
+    name === nothing && return expr
+    ti = TestItemMetadata(name, tags)
+    return f(ti) ? expr : nothing
+end
 
 # Check if the expression is a macrocall as expected.
 # NOTE: Only `@testitem` and `@testsetup` calls are officially supported.

src/testcontext.jl

@@ -32,29 +32,16 @@ mutable struct TestContext
     TestContext(name, ntestitems) = new(name, ntestitems)
 end
 
-# FilteredVector applies a filtering function `f` to items
-# when you try to `push!` and only puts if `f` returns true.
-# TODO: drop this when all filtering is done at the AST level
-# i.e. when we drop support for _RAI_MACRO_NAME
-struct FilteredVector{T} <: AbstractVector{T}
-    f::Any
-    vec::Vector{T}
-end
-
-Base.push!(x::FilteredVector, y) = x.f(y) && push!(x.vec, y)
-Base.size(x::FilteredVector) = size(x.vec)
-Base.getindex(x::FilteredVector, i) = x.vec[i]
-
 struct FileNode
     path::String
     testset::DefaultTestSet
     junit::Union{JUnitTestSuite,Nothing}
-    testitems::FilteredVector{TestItem} # sorted by line number within file
+    testitems::Vector{TestItem} # sorted by line number within file
 end
 
 function FileNode(path, f=Returns(true); report::Bool=false, verbose::Bool=false)
     junit = report ? JUnitTestSuite(path) : nothing
-    return FileNode(path, DefaultTestSet(path; verbose), junit, FilteredVector(f, TestItem[]))
+    return FileNode(path, DefaultTestSet(path; verbose), junit, TestItem[])
 end
 
 Base.push!(f::FileNode, ti::TestItem) = push!(f.testitems, ti)

test/integrationtests.jl

@@ -835,12 +835,17 @@ end
         @assert ReTestItems.___RAI_MACRO_NAME_DONT_USE == Symbol("@test_rel")
         @eval begin
             macro test_rel(args...)
-                if length(args) == 1
-                    name = "anon"
-                    ex = args[1]
-                else
-                    name = args[1]
-                    ex = args[2]
+                local name::String
+                local ex::Expr
+                local tags = Symbol[]
+                for arg in args
+                    if arg.head == :(=) && arg.args[1] == :name
+                        name = arg.args[2]
+                    elseif arg.head == :(=) && arg.args[1] == :tags
+                        tags = arg.args[2]
+                    elseif arg.head == :(=) && arg.args[1] == :code
+                        ex = arg.args[2]
+                    end
                 end
                 quote
                     @testitem $(name) begin
@@ -856,7 +861,9 @@ end
         @test n_tests(results) == 3
         results = encased_testset(() -> runtests(file; name="ti"))
         @test n_tests(results) == 1
-        results = encased_testset(() -> runtests(file; name="anon"))
+        results = encased_testset(() -> runtests(file; name=r"other"))
+        @test n_tests(results) == 2
+        results = encased_testset(() -> runtests(file; tags=[:xyz]))
         @test n_tests(results) == 1
         results = encased_testset(() -> runtests(filter_func, file))
         @test n_tests(results) == 0

test/internals.jl

@@ -328,13 +328,10 @@ end
         using ReTestItems: filter_testitem
         ti = :(@testitem "TI" tags=[:foo, :bar] begin; @test true; end)
         ts = :(@testsetup module TS; x = 1; end)
-        tx = :(@other_macro "TX" tags=[:foo, :bar] begin; @test true; end)
         @test filter_testitem(Returns(true),  ti) == ti
         @test filter_testitem(Returns(false), ti) == nothing
         @test filter_testitem(Returns(true),  ts) == ts
         @test filter_testitem(Returns(false), ts) == ts
-        @test filter_testitem(Returns(true),  tx) == tx
-        @test filter_testitem(Returns(false), tx) == tx
     end
     @testset "try_get_name" begin
         using ReTestItems: try_get_name

test/testfiles/_support_rai_test.jl

@@ -10,10 +10,10 @@ end
 end
 
 # this macro is defined in `integrationtests.jl` where this file is run.
-@test_rel begin
+@test_rel(name="other A", code=begin
     @test 1 == 1
-end
+end)
 
-@test_rel "other" begin
-    @test 1 == 1
-end
+@test_rel(tags=[:xyz], name="other B", code=begin
+    @test 2 == 2
+end)