Allow for duplicate properties

docs/src/manual.md

@@ -98,9 +98,9 @@ The added properties will be added to the corresponding testsuite in the generat
 Multiple properties can be added, and a property added to a parent `TestSet` will be applied
 to all child `TestSet`s.
 
-An error will be thrown if the same property is set twice in a `TestSet`, and a warning
-displayed if both parent and child `TestSet` have the same property set (in which case
-the value set in the child will take be used in the report).
+Properties with the same name are allowed to be set multiple times within the same
+`TestSet`. If both a parent and a child set the same named property both properties will
+appear in the child when generating the report.
 
 The property name must always be a `String`, but the value can be anything that is serializable
 by `EzXML.jl`. In practice this means that `String`s, `Number`s, `Expr`s and `Symbols` can be used,
@@ -120,13 +120,13 @@ using TestReports
         recordproperty("TestSubject", "Example")
 
         @testset "Inner" begin
-            recordproperty("Testsuite", 101)  # This will overwrite parent testset value
-            @test 1==1
+            recordproperty("Testsuite", 101)  # Both testsuite 100 and 101 apply to this testset
+            @test 1 == 1
         end
 
         @testset "Types" begin
             recordproperty("Prop1", :Val1)
-            @test 1==1
+            @test 1 == 1
         end
     end
 end

src/recordproperty.jl

@@ -1,26 +1,27 @@
 """
-    recordproperty(name::String, value)
+    recordproperty(name::AbstractString, value)
 
-Adds a property to a testset with `name` and `value` that will in turn be added
-to the `<properties>` node of the corresponding testsuite in the JUnit XML.
+Associates a property with a testset. The `name` and `value` will be turned into a
+`<property>` element within the corresponding `<testsuite>` element within the JUnit XML
+report.
 
-Multiple properties can be added to one testset, but if the same property is set on
-both parent and child testsets, the value in the child testset takes precedence over
-that in the parent.
+Multiple properties can be added to one testset and child testsets inherit the properties
+defined by their parents. If a child testset records a property which is already set both
+will be present in the resulting report.
 
 The suggested use of this function is to place it inside a testset with unspecified type
 (see Examples). This will ensure that `Pkg.test` is unnaffected, but that the properties
 are added to the report when `TestReports.test` is used. This is because properties are
 only added when the `Testset` type has a `TestReports.properties` method defined, as does
-the `ReportingTestSet` used by `TestReports`. `TestReports.properties` can be extended
-for custom `TestSet`s.
+the `ReportingTestSet` used by `TestReports`. `TestReports.properties` can be extended for
+custom `TestSet`s.
 
 If a child testset has this method defined but its parent doesn't, the property should
 be in the report when `TestReport.test` is used, assuming that the parent testset
-type doesn't do anything to affect the reporting behaviour. However this is not tested
+type doesn't do anything to affect the reporting behavior. However this is not tested
 functionality.
 
-`value` must be serializable by EzXML, which gives quite a lot of freedom.
+The `value` argument must be serializable by EzXML, which gives quite a lot of freedom.
 
 # Examples
 ```julia
@@ -32,22 +33,14 @@ using TestReports
     recordproperty("ID", 42)
     recordproperty("File", @__FILE__)
     recordproperty("Bool", true)
-    @test 1==1
-    @test 2==2
+    @test 1 == 1
+    @test 2 == 2
 end
 ```
 
-See also: [`properties`](@ref)
+See also: [`properties`](@ref) and [`recordproperty!](@ref).
 """
-function recordproperty(name::String, val)
-    properties_dict = properties(get_testset())
-    if !isnothing(properties_dict)
-        !isa(properties_dict, AbstractDict) && throw(PkgTestError("TestReports.properties method for custom testset must return a dictionary."))
-        if haskey(properties_dict, name)
-            throw(PkgTestError("Property $name already set and can't be set again in the same testset"))
-        else
-            get_testset().properties["$name"] = val
-        end
-    end
-    return
-end
+function recordproperty(name::AbstractString, value)
+    recordproperty!(get_testset(), name, value)
+    return value
+end

src/testsets.jl

@@ -56,8 +56,7 @@ and this is not recommended or supported.
 A `ReportingTestSet` has the `description` and `results` fields as per a
 `DefaultTestSet`, and has the following additional fields:
 
-- `properties`: a dictionary which is used to record properties to be
-    inserted into the report.
+- `properties`: used to record testsuite properties to be inserted into the report.
 - `start_time::DateTime`: the start date and time of the testing (local system time).
 - `time_taken::Millisecond`: the time taken in milliseconds to run the `TestSet`.
 - `last_record_time::DateTime`: the time when `record` was last called.
@@ -68,7 +67,7 @@ See also: [`flatten_results!`](@ref), [`recordproperty`](@ref), [`report`](@ref)
 mutable struct ReportingTestSet <: AbstractTestSet
     description::String
     results::Vector
-    properties::Dict{String, Any}
+    properties::Vector{Pair{String, Any}}
     start_time::DateTime
     time_taken::Millisecond
     last_record_time::DateTime
@@ -77,7 +76,8 @@ mutable struct ReportingTestSet <: AbstractTestSet
     showtiming::Bool
 end
 
-ReportingTestSet(desc; verbose=false, showtiming=true) = ReportingTestSet(desc, [], Dict(), now(), Millisecond(0), now(), gethostname(), verbose, showtiming)
+ReportingTestSet(desc; verbose=false, showtiming=true) = ReportingTestSet(desc, [], [], now(), Millisecond(0), now(), gethostname(), verbose, showtiming)
+
 function record(ts::ReportingTestSet, t::Result)
     push!(ts.results, ReportingResult(t, now()-ts.last_record_time))
     ts.last_record_time = now()
@@ -111,16 +111,31 @@ end
 #################################
 # Accessing and setting methods #
 #################################
+
 """
-    properties(ts::ReportingTestSet)
-    properties(ts::AbstractTestSet)
+    properties(ts::AbstractTestSet) -> Union{Vector{Pair{String, Any}}, Nothing}
 
-Get the properties dictionary of a `ReportingTestSet`, returns
-nothing for an `AbstractTestSet`. Can be extended for custom
-`TestSet`s, and must return either a `Dict` or `nothing`.
+Get the properties of a `ReportingTestSet`. Can be extended for custom testsets.
+Returns `nothing` for an `AbstractTestSet`.
+
+See also: [`recordproperty`](@ref) and [`recordproperty!`](@ref).
 """
+properties(::AbstractTestSet) = nothing
 properties(ts::ReportingTestSet) = ts.properties
-properties(ts::AbstractTestSet) = nothing
+
+"""
+    recordproperty!(ts::AbstractTestSet, name::AbstractString, value)
+
+Associate a property with the testset. Can be extended for custom testsets.
+
+See also: [`properties`](@ref).
+"""
+recordproperty!(ts::AbstractTestSet, name::AbstractString, value) = ts
+
+function recordproperty!(ts::ReportingTestSet, name::AbstractString, value)
+    push!(ts.properties, name => value)
+    return ts
+end
 
 """
     start_time(ts::ReportingTestSet)
@@ -275,36 +290,32 @@ _flatten_results!(rs::Result, depth::Int) = [rs]
 """
     update_testset_properties!(childts::AbstractTestSet, ts::AbstractTestSet)
 
-Adds properties of `ts` to `childts`. If any properties being added already exist in
-`childts`, a warning is displayed and the value in `ts` is overwritten.
+Adds properties of `ts` to `childts`.
+
+If the types of `ts` and/or `childts` do not a method defined for properties, this is
+handled as follows:
 
-If the types of `ts` and\\or `childts` do not a method defined for `TestReports.properties`,
-this is handled as follows:
 - If method not defined for `typeof(ts)`, it has no properties to add to `childts`
-    and therefore nothing happens.
-- If method not defined for `typeof(chidlts)` and `ts` has properties, then a warning
-    is shown.
+  and therefore nothing happens.
+- If method not defined for `typeof(childts)` and `ts` has properties, then a warning is
+  shown.
 
-See also: [`properties`](@ref)
+See also: [`properties`](@ref).
 """
 function update_testset_properties!(childts::AbstractTestSet, ts::AbstractTestSet)
     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(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
-                child_props[key] = parent_props[key]
-            end
+    # Children inherit the properties of their parents
+    if !isnothing(parent_props) && !isempty(parent_props)
+        if !isnothing(child_props)
+            prepend!(child_props, parent_props)
+        else
+            @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."
+            )
         end
     end
     return childts