Flatten testsets during report generation (#104)

* Flattened testsets ordered by appearance * Flatten during report generation Refactored the code to no longer flatten at the top-level when finishing the top-level `ReportingTestSet` and instead perform the flattening when we generate the JUnit XML report. Doing this allows introspecting of the nested `ReportingTestSet` structure and opens up the possibility of using this testset in other report generation which does support nesting. * Consider the default testset name as unnamed * Allow CI tests to work on all PR base branches
JuliaTesting · May 1, 2024 · 4111d2c · 4111d2c
1 parent cde234f
commit 4111d2c
Show file tree

Hide file tree

Showing 7 changed files with 123 additions and 143 deletions.
diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml
@@ -1,8 +1,6 @@
 name: CI
 on:
   pull_request:
-    branches:
-      - master
   push:
     branches:
       - master

diff --git a/src/testsets.jl b/src/testsets.jl
@@ -105,8 +105,7 @@ function finish(ts::ReportingTestSet)
     # Display before flattening to match Pkg.test output
     display_reporting_testset(ts)
 
-    # We are the top level, lets do this
-    flatten_results!(ts)
+    return ts
 end
 
 #################################
@@ -205,31 +204,28 @@ any_problems(::Error) = true
 #####################
 
 """
-    flatten_results!(ts::AbstractTestSet)
+    has_description(ts::AbstractTestSet) -> Bool
 
-Returns a flat structure 3 deep, of `TestSet` -> `TestSet` -> `Result`. This is necessary
-for writing a report, as a JUnit XML does not allow one testsuite to be nested in another.
-The top level `TestSet` becomes the testsuites element, and the middle level `TestSet`s
-become individual testsuite elements, and the `Result`s become the testcase elements.
+Determine if the testset has been provided a description.
+"""
+function has_description(ts::AbstractTestSet)
+    !isempty(ts.description) && ts.description != "test set"
+end
 
-If `ts.results` contains any `Result`s, these are added into a new `TestSet` with the
-description "Top level tests", which then replaces them in `ts.results`.
 """
-function flatten_results!(ts::AbstractTestSet)
-    # Add any top level Results to their own TestSet
-    handle_top_level_results!(ts)
+    flatten_results!(ts::AbstractTestSet)
 
-    # Flatten all results of top level testset, which should all be testsets now
-    ts.results = vcat(_flatten_results!.(ts.results)...)
-    return ts
-end
+Returns a flat vector of `TestSet`s which only contain `Result`s. This is necessary for
+writing a JUnit XML report the schema does not allow nested XML `testsuite` elements.
+"""
+flatten_results!(ts::AbstractTestSet) = _flatten_results!(ts, 0)
 
 """
     _flatten_results!(ts::AbstractTestSet)::Vector{<:AbstractTestSet}
 
 Recursively flatten `ts` to a vector of `TestSet`s.
 """
-function _flatten_results!(ts::AbstractTestSet)::Vector{<:AbstractTestSet}
+function _flatten_results!(ts::AbstractTestSet, depth::Int)::Vector{AbstractTestSet}
     original_results = ts.results
     has_new_properties = !isempty(something(properties(ts), tuple()))
     flattened_results = AbstractTestSet[]
@@ -245,13 +241,15 @@ function _flatten_results!(ts::AbstractTestSet)::Vector{<:AbstractTestSet}
     function inner!(childts::AbstractTestSet)
         # Make it a sibling
         update_testset_properties!(childts, ts)
-        childts.description = ts.description * "/" * childts.description
+        if depth > 0 || has_description(ts)
+            childts.description = ts.description * "/" * childts.description
+        end
         push!(flattened_results, childts)
     end
 
     # Iterate through original_results
     for res in original_results
-        childs = _flatten_results!(res)
+        childs = _flatten_results!(res, depth + 1)
         for child in childs
             inner!(child)
         end
@@ -272,7 +270,7 @@ end
 Return vector containing `rs` so that when iterated through,
 `rs` is added to the results vector.
 """
-_flatten_results!(rs::Result) = [rs]
+_flatten_results!(rs::Result, depth::Int) = [rs]
 
 """
     update_testset_properties!(childts::AbstractTestSet, ts::AbstractTestSet)
@@ -290,51 +288,28 @@ this is handled as follows:
 See also: [`properties`](@ref)
 """
 function update_testset_properties!(childts::AbstractTestSet, ts::AbstractTestSet)
-    if isnothing(properties(childts)) && !isnothing(properties(ts)) && !isempty(properties(ts))
+    child_props = properties(childts)
+    parent_props = properties(ts)
+
+    if isnothing(child_props) && !isnothing(parent_props) && !isempty(parent_props)
         @warn "Properties of testset $(ts.description) can not be added to child testset $(childts.description) as it does not have a TestReports.properties method defined."
         # No need to check if childts is has properties defined and ts doesn't as if this is the case
         # ts has no properties to add to that of childts.
-    elseif !isnothing(properties(ts))
-        parent_keys = keys(properties(ts))
-        child_keys = keys(properties(childts))
+    elseif !isnothing(child_props) && !isnothing(parent_props)
+        parent_keys = keys(parent_props)
+        child_keys = keys(child_props)
         # Loop through keys so that warnings can be issued for any duplicates
         for key in parent_keys
             if key in child_keys
                 @warn "Property $key in testest $(ts.description) overwritten by child testset $(childts.description)"
             else
-                properties(childts)[key] = properties(ts)[key]
+                child_props[key] = parent_props[key]
             end
         end
     end
     return childts
 end
 
-"""
-    handle_top_level_results!(ts::AbstractTestSet)
-
-If `ts.results` contains any `Result`s, these are removed from `ts.results` and
-added to a new `ReportingTestSet`, which in turn is added to `ts.results`. This
-leaves `ts.results` only containing `AbstractTestSet`s.
-
-The `time_taken` field of the new `ReportingTestSet` is calculated by summing
-the time taken by the individual results, and the `start_time` field is set to
-the `start_time` field of `ts`.
-"""
-function handle_top_level_results!(ts::AbstractTestSet)
-    isa_Result = isa.(ts.results, Result)
-    if any(isa_Result)
-        original_results = ts.results
-        ts.results = AbstractTestSet[]
-        ts_nested = ReportingTestSet("Top level tests")
-        ts_nested.results = original_results[isa_Result]
-        set_time_taken!(ts_nested, sum(x -> time_taken(x)::Millisecond, ts_nested.results))
-        set_start_time!(ts_nested, start_time(ts)::DateTime)
-        push!(ts.results, ts_nested)
-        append!(ts.results, original_results[.!isa_Result])
-    end
-    return ts
-end
-
 """
     display_reporting_testset(ts::ReportingTestSet)
 

diff --git a/src/to_xml.jl b/src/to_xml.jl
@@ -120,30 +120,29 @@ end
 #####################
 
 """
-    report(ts::AbstractTestSet)
+    report(ts::AbstractTestSet) -> XMLDocument
 
-Will produce an `XMLDocument` that contains a report about the `TestSet`'s results.
-This report will follow the JUnit XML schema.
+Produce an JUnit XML report details about the contained `TestSet`s and `Result`s. As the
+JUnit XML schema does not allow nested `testsuite` elements the report will flatten the
+hierarchical `TestSet` structure. Each `TestSet` will become a `testsuite` element and each
+`Result` will become a `testcase` element.
 
-To report correctly, the `TestSet` must have the following structure:
+A `Result` will only be reported once within its parent `TestSet` to avoid having duplicate
+entries within the report and avoid problems with total test counts not matching Julia
+output.
 
-    AbstractTestSet
-      └─ AbstractTestSet
-           └─ AbstractResult
-
-That is, the results of the top level `TestSet` must all be `AbstractTestSet`s,
-and the results of those `TestSet`s must all be `Result`s.
-
-Additionally, all of the `AbstractTestSet`s must have both `description` and
-`results` fields.
+All `AbstractTestSet`s contained within `ts` must have a `description::AbstractString` field
+and an iterable `results` field.
 """
-function report(ts::AbstractTestSet)
-    check_ts(ts)
+report(ts::AbstractTestSet) = report(flatten_results!(deepcopy(ts)))
+
+function report(testsets::Vector{<:AbstractTestSet})
+    check_ts(testsets)
     total_ntests = 0
     total_nfails = 0
     total_nerrors = 0
     testsuiteid = 0 # ID increments from 0
-    x_testsuites = map(ts.results) do result
+    x_testsuites = map(testsets) do result
         x_testsuite, ntests, nfails, nerrors = to_xml(result)
         total_ntests += ntests
         total_nfails += nfails
@@ -159,7 +158,7 @@ function report(ts::AbstractTestSet)
                                          total_nerrors,
                                          x_testsuites))
 
-    xdoc
+    return xdoc
 end
 
 """
@@ -170,11 +169,13 @@ the results of `ts` do not have both `description` or `results` fields.
 
 See also: [`report`](@ref)
 """
-function check_ts(ts::AbstractTestSet)
-    !all(isa.(ts.results, AbstractTestSet)) && throw(ArgumentError("Results of ts must all be AbstractTestSets. See documentation for `report`."))
-    for results_ts in ts.results
-        !isa(results_ts.description, AbstractString) && throw(ArgumentError("description field of $(typeof(results_ts)) must be an AbstractString."))
-        !all(isa.(results_ts.results, Result)) && throw(ArgumentError("Results of each AbstractTestSet in ts.results must all be Results. See documentation for `report`."))
+function check_ts(testsets::Vector{<:AbstractTestSet})
+    for ts in testsets
+        if !isa(ts.description, AbstractString)
+            throw(ArgumentError("description field of $(typeof(ts)) must be an `AbstractString`."))
+        elseif !all(r -> r isa Result, ts.results)
+            throw(ArgumentError("Results of each `AbstractTestSet` in ts.results must all be `Result`s. See documentation for `report`."))
+        end
     end
 end
 
@@ -342,4 +343,4 @@ function add_testsuite_properties!(x_testsuite, ts::AbstractTestSet)
         link!(x_testsuite, x_properties)
     end
     return x_testsuite
-end
+end
diff --git a/test/example.jl b/test/example.jl
@@ -1,6 +1,6 @@
 using Test
 using TestReports
 
-(@testset ReportingTestSet "Example" begin
+(@testset ReportingTestSet "" begin
     include("example_normaltestsets.jl")
 end) |> report |> println
diff --git a/test/recordproperty.jl b/test/recordproperty.jl
@@ -94,8 +94,9 @@
         end
         # Force flattening as ts doesn't finish fully as it is not the top level testset
         overwrite_text = "Property ID in testest Outer overwritten by child testset Inner"
-        @test_logs (:warn, overwrite_text) TestReports.flatten_results!(ts)
-        @test ts.results[2].properties["ID"] == "0"
+        flattened_testsets = @test_logs (:warn, overwrite_text) TestReports.flatten_results!(ts)
+        @test length(flattened_testsets) == 2
+        @test flattened_testsets[2].properties["ID"] == "0"
 
         # Test for parent testset properties not being applied to child due to different type
         ts = @testset ReportingTestSet "" begin
@@ -120,8 +121,9 @@
             end
         end
         # Force flattening as ts doesn't finish fully as it is not the top level testset
-        TestReports.flatten_results!(ts)
-        @test ts.results[1].properties["ID"] == "42"
+        flattened_testsets = TestReports.flatten_results!(ts)
+        @test length(flattened_testsets) == 1
+        @test flattened_testsets[1].properties["ID"] == "42"
 
         # Error if attempting to add property to AbstractTestSet which has properties field with wrong type
         ts = @testset WrongPropsTestSet begin; recordproperty("id",1); end

diff --git a/test/reportgeneration.jl b/test/reportgeneration.jl
@@ -8,7 +8,7 @@ const TEST_PKG = (name = "Example", uuid = UUID("7876af07-990d-54b4-ab0e-2369062
 
 @testset "SingleNest" begin
     test_file = VERSION >= v"1.7.0" ? "references/singlenest.txt" : "references/singlenest_pre_1_7.txt"
-    @test_reference test_file read(`$(Base.julia_cmd()) -e "using Test; using TestReports; (@testset ReportingTestSet \"blah\" begin @testset \"a\" begin @test 1 ==1 end end) |> report |> print"`, String) |>  clean_output
+    @test_reference test_file read(`$(Base.julia_cmd()) -e "using Test; using TestReports; (@testset ReportingTestSet \"\" begin @testset \"a\" begin @test 1 == 1 end end) |> report |> print"`, String) |>  clean_output
 end
 
 @testset "Complex Example" begin
@@ -148,46 +148,32 @@ end
 end
 
 @testset "report - check_ts" begin
-    # No top level testset
-    ts = @testset TestReportingTestSet begin
-        @test true
-    end
-    @test_throws ArgumentError TestReports.check_ts(ts)
-
     # Not flattened
     ts = @testset TestReportingTestSet begin
+        @test true
         @testset TestReportingTestSet begin
             @test true
-            @testset TestReportingTestSet begin
-                @test true
-            end
         end
     end
-    @test_throws ArgumentError TestReports.check_ts(ts)
+    @test_throws ArgumentError TestReports.check_ts([ts])
 
     # No description field
-    ts = @testset TestReportingTestSet begin
-        @testset NoDescriptionTestSet begin
-            @test true
-        end
+    ts = @testset NoDescriptionTestSet begin
+        @test true
     end
-    @test_throws ErrorException TestReports.check_ts(ts)
+    @test_throws ErrorException TestReports.check_ts([ts])
 
     # No results field
-    ts = @testset TestReportingTestSet begin
-        @testset NoResultsTestSet begin
-            @test true
-        end
+    ts = @testset NoResultsTestSet begin
+        @test true
     end
-    @test_throws ErrorException TestReports.check_ts(ts)
+    @test_throws ErrorException TestReports.check_ts([ts])
 
     # Correct structure
     ts = @testset TestReportingTestSet begin
-        @testset TestReportingTestSet begin
-            @test true
-        end
+        @test true
     end
-    @test TestReports.check_ts(ts) isa Any # Doesn't error
+    @test TestReports.check_ts([ts]) isa Any # Doesn't error
 end
 
 @testset "Error counting - Issue #72" begin
@@ -196,7 +182,7 @@ end
             variableThatDoNotExits # No test here so shouldn't count
         end
         @testset "test_error" begin
-            @test variableThatDoNotExits == 42     
+            @test variableThatDoNotExits == 42
         end
         @testset "test_unbroken" begin
             @test_broken true