Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support testcase properties #106

Merged
merged 5 commits into from
May 21, 2024
Merged

Support testcase properties #106

merged 5 commits into from
May 21, 2024

Conversation

omus
Copy link
Member

@omus omus commented Apr 29, 2024

Adds support for defining testcase properties. Both #103 and #104 are precursors to this work.

Overview of the work done here:

  • Removes recordproperty in favor of record_testset_property. I believe recordproperty was inspired from the pytest fixture record_property which is supposed to add properties to testcases.
  • Adds record_test_property to allow associating properties against Julia test results. I primarily require this functionality to support this tool. The JUnit XML schema doesn't officially support properties on testcases but other JUnit reporting tools already support testcase properties. Test properties defined on testsets which contain no dependent tests are ignored.

@omus omus marked this pull request as draft April 30, 2024 15:29
Base automatically changed from cv/flatten-refactor to master May 1, 2024 14:16
src/deprecated.jl Outdated Show resolved Hide resolved
@omus omus marked this pull request as ready for review May 1, 2024 14:35
@omus omus requested a review from oxinabox May 1, 2024 14:35
@oxinabox
Copy link
Member

oxinabox commented May 1, 2024

It is unclear to me from the documentation changed in this PR what the actual difference between a testset_property and test_property is.

Without understanding that it is hard to review

@omus
Copy link
Member Author

omus commented May 1, 2024

It is unclear to me from the documentation changed in this PR what the actual difference between a testset_property and test_property is.

Without understanding that it is hard to review

I've added some additional details to those docstrings in terms of how they get translated into the JUnit XML report. Hopefully, that addressed your concern. Please feel free to ask additional questions if you're still having a hard time reviewing.

@@ -141,6 +146,11 @@ end

Get the properties associated with tests within a testset. Can be extended for custom
testsets. Will return `nothing` for testsets which do not support test properties.

When generating a JUnit XML report these will be the properties associated with all
`testcase` elements contained within a `testsuite` element.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So they will be duplicated?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No. The properties created through record_testset_property are associated to the testset containing the call and all testsets nested within that testset. The properties created through record_test_property are associated with the tests contained within the testset containing the function call including all nested tests. When rendering the JUnit report this means that calls to record_testset_property are associated with <testsuite> elements while calls to record_test_property are associated with <testcase> elements.

For example:

using Test, TestReports, EzXML

ts = @testset ReportingTestSet "Root" begin
    record_testset_property("foo", 1)
    record_test_property("bar", 2)

    @testset "Inner" begin
        @test 1 == 1
    end

    @test 2 != 1
end;

prettyprint(report(ts))

Results in:

<?xml version="1.0" encoding="UTF-8"?>
<testsuites tests="2" failures="0" errors="0">
  <testsuite name="Root" tests="1" failures="0" errors="0" time="0.080" timestamp="2024-05-02T09:00:58.607" hostname="awro" id="0">
    <testcase name="2 != 1" id="1" classname="Root" time="0.000">
      <properties>
        <property name="bar" value="2"/>
      </properties>
    </testcase>
    <properties>
      <property name="foo" value="1"/>
    </properties>
  </testsuite>
  <testsuite name="Root/Inner" tests="1" failures="0" errors="0" time="0.053" timestamp="2024-05-02T09:00:58.634" hostname="awro" id="1">
    <testcase name="1 == 1" id="1" classname="Root/Inner" time="0.011">
      <properties>
        <property name="bar" value="2"/>
      </properties>
    </testcase>
    <properties>
      <property name="foo" value="1"/>
    </properties>
  </testsuite>
</testsuites>

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it. this clarified things a lot.
Can this be added into the docs somewhere?
Either in the docstring or in the manual?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've updated the examples in the docstring for record_testset_properties to include some of these details. I ended up not including the raw XML output as I thought it was getting a bit too verbose. Hopefully, the example is as clear as this comment. Let me know what you think

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll merge this PR with the updated docstring. If you think it could be made clearer I can make another PR.

@omus
Copy link
Member Author

omus commented May 17, 2024

Rebased this PR since the code in #110 was extracted from this PR. Hopefully, this PR is easier to read now. The test/properties.jl file unfortunately doesn't have a proper diff due to the file rename and additional modifications

@omus omus mentioned this pull request May 17, 2024
@omus omus requested a review from oxinabox May 17, 2024 17:56
Copy link
Member

@oxinabox oxinabox left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, cool now i understand it.
And this looks good to me.
Thank you for breaking this up.

Just one small comment.
Then once this is in and the other one we will do a 1.0 release

@omus omus merged commit 8250050 into master May 21, 2024
10 checks passed
@omus omus deleted the cv/test-properties branch May 21, 2024 18:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants