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

Polarion support #243

Merged
merged 6 commits into from
Jun 19, 2019
Merged

Conversation

clacroix12
Copy link
Contributor

Added the ability to mark tests with their respective polarion_ids in order to support posting our test results to Polarion. When we export our test results to a junit xml file (--junit-xml report.xml) any test cases that are marked with polarion_ids will have a custom property set that reflects the value.

Ex.

<properties><property name="polarion-testcase-id" value="OCS-foo"/></properties>

Note that these changes do not add the ability to post the results to Polarion, only to support the tool we will use to take our xml test results and post the results.

@clacroix12 clacroix12 requested a review from a team as a code owner June 17, 2019 22:45
@clacroix12 clacroix12 mentioned this pull request Jun 17, 2019
2 tasks
@clacroix12 clacroix12 added enhancement New feature or request High Priority High priority issues labels Jun 17, 2019
mbukatov
mbukatov previously approved these changes Jun 18, 2019
Copy link
Contributor

@mbukatov mbukatov left a comment

Choose a reason for hiding this comment

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

This looks good.

Note that polarion-project-id property is mandatory for testsuites element (as noted in docs of polarion ci importer). But the junit xml report starts with testsuite element:

(venv) [ocsqe@localhost ocs-ci]$ xmllint --format report.xml
<?xml version="1.0" encoding="utf-8"?>
<testsuite errors="0" failures="1" name="pytest" skipped="0" tests="2" time="0.125">
  <testcase classname="tests.test_foo" file="tests/test_foo.py" line="2" name="test_foo_bar" time="0.002">
    <properties>
      <property name="polarion-testcase-id" value="FOO-123"/>
    </properties>
  </testcase>
  <testcase classname="tests.test_foo" file="tests/test_foo.py" line="6" name="test_foo_baz" time="0.001">
    <properties>
      <property name="polarion-testcase-id" value="FOO-120"/>
    </properties>
    <failure message="assert 1 == 0   -1   +0">@pytest.mark.polarion_id("FOO-120")
    def test_foo_baz():
&gt;       assert 1 == 0
E       assert 1 == 0
E         -1
E         +0

tests/test_foo.py:9: AssertionError</failure>
  </testcase>
</testsuite>

So the question is if importer will recognize polarion-project-id property in testsuite element (which we could provide in pytest), or whether we need to wrap the report into testsuites element and add the project id there (which could be done in the CI job pushing the report into the importer).

That said, I'm ok with letting this in, as it's not clear at which point we should add polarion-project-id property.

dahorak
dahorak previously approved these changes Jun 18, 2019
vasukulkarni
vasukulkarni previously approved these changes Jun 18, 2019
Add Polarion ID property to test cases that are marked with one.
"""
for item in items:
for marker in item.iter_markers(name="polarion_id"):
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reiterating what we discussed earlier: I think get_closest_marker is a good idea. This could save us in the event that both a test method and the test class it resides in are marked with polarion_ids that conflict. With get_closest_marker we would only use the value from the test level marker and not the class. This is still a situation we don't want to have present in our tests, but we also don't want to post results to the wrong tests in the event it happens. +1 for get_closest_marker.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Copy link
Contributor

@dahorak dahorak Jun 19, 2019

Choose a reason for hiding this comment

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

What if one automated test covers more test cases in Polarion? (not sure, if it is "allowed" or not, but I think, it is possible to have such case...)

Previously it was possible to use something like:

  @pytest.mark.polarion_id("FOO1")
  @pytest.mark.polarion_id("FOO2")
  @pytest.mark.polarion_id("FOO3")
  def test_foo(self):
      ...

But now, it will use only the last ID (FOO3).

On the other side it doesn't make sense to add Polarion ID on class level, because it doesn't make sense to have more automated tests reporting to one test case in Polarion...

Copy link
Contributor

Choose a reason for hiding this comment

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

@dahorak I'm not sure if the importer will be able to handle this anyway.

for item in items:
for marker in item.iter_markers(name="polarion_id"):
polarion_id = marker.args[0]
item.user_properties.append(("polarion-testcase-id", polarion_id))
Copy link
Member

Choose a reason for hiding this comment

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

We will need to add one more global required parameter under testsuites to junit file. See the documentation:
https://mojo.redhat.com/docs/DOC-1073077#jive_content_id_polarionprojectid_required

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, but I would rather not add it for every test case individually.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah like @mbukatov said in his top level comment we might need to wrap the output from our tests in a <testsuites> element as it doesn't exist in the report by default. The property you linked to needs to be a property of <testsuites> not the <testcase> so I wouldn't add it in this loop.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, I've just dig in to how it's done in rhev and you can see one of generated xunit file which works for polarion importer in RHV automation here: https://rhv-jenkins.rhev-ci-vms.eng.rdu2.redhat.com/view/4.3/job/rhv-4.3-ge-runner-tier1/81/artifact/xunit_output.xml

I see that it can be in testusite under properties as you can see here

<testsuite errors="0" failures="3" name="pytest" skips="42" tests="706" time="68101.387">
    <properties>
        <property name="polarion-custom-isautomated" value="True"/>
        <property name="polarion-testrun-status-id" value="inprogress"/><property name="polarion-custom-plannedin" value="RHV_4_3"/>
        <property name="polarion-user-id" value="ci-user"/>
        <property name="polarion-project-id" value="RHEVM3"/><property name="polarion-response-myproduct" value="rhvm"/>
        <property name="polarion-testrun-id" value="RHV_4_3_tier1_x86_64"/><property name="polarion-custom-arch" value="x8664"/>
    </properties>
    <testcase classname="rhevmtests.compute.sla.cpu_qos.cpu_qos_test.TestQoSAndCpuProfileCRUD" file="rhevmtests/compute/sla/cpu_qos/cpu_qos_test.py" line="60" name="test_a_add_qos" time="11.999502182"><properties><property name="polarion-testcase-id" value="RHEVM-14931"/></properties>
        <system-out>
...output omitted...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added a session fixture to handle adding these properties. For now it only adds the polarion-project-id but we can update it to add any others we might need.

Copy link
Contributor

Choose a reason for hiding this comment

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

Good.

@vasukulkarni
Copy link
Contributor

This looks good.

Note that polarion-project-id property is mandatory for testsuites element (as noted in docs of polarion ci importer). But the junit xml report starts with testsuite element:

(venv) [ocsqe@localhost ocs-ci]$ xmllint --format report.xml
<?xml version="1.0" encoding="utf-8"?>
<testsuite errors="0" failures="1" name="pytest" skipped="0" tests="2" time="0.125">
  <testcase classname="tests.test_foo" file="tests/test_foo.py" line="2" name="test_foo_bar" time="0.002">
    <properties>
      <property name="polarion-testcase-id" value="FOO-123"/>
    </properties>
  </testcase>
  <testcase classname="tests.test_foo" file="tests/test_foo.py" line="6" name="test_foo_baz" time="0.001">
    <properties>
      <property name="polarion-testcase-id" value="FOO-120"/>
    </properties>
    <failure message="assert 1 == 0   -1   +0">@pytest.mark.polarion_id("FOO-120")
    def test_foo_baz():
&gt;       assert 1 == 0
E       assert 1 == 0
E         -1
E         +0

tests/test_foo.py:9: AssertionError</failure>
  </testcase>
</testsuite>

So the question is if importer will recognize polarion-project-id property in testsuite element (which we could provide in pytest), or whether we need to wrap the report into testsuites element and add the project id there (which could be done in the CI job pushing the report into the importer).

That said, I'm ok with letting this in, as it's not clear at which point we should add polarion-project-id property.

project-id and test-run-id can be set at jenkins level? since they are just one time per testsuite?

@clacroix12
Copy link
Contributor Author

project-id and test-run-id can be set at jenkins level? since they are just one time per testsuite?

@vasukulkarni That's one possibility as we will already need to wrap the junit.xml in a <testsuites> element anyways. We might as well handle the <testsuites> level properties in the same operation. The other possibility is we format it and add the properties ourselves as a session level finalizer if we have passed the CLI args to export the junit.xml results. Either of which I think can be addressed outside of this PR once we have made some progress with running the actual importer.

@petr-balogh
Copy link
Member

project-id and test-run-id can be set at jenkins level? since they are just one time per testsuite?

@vasukulkarni That's one possibility as we will already need to wrap the junit.xml in a <testsuites> element anyways. We might as well handle the <testsuites> level properties in the same operation. The other possibility is we format it and add the properties ourselves as a session level finalizer if we have passed the CLI args to export the junit.xml results. Either of which I think can be addressed outside of this PR once we have made some progress with running the actual importer.

I prefer to prepare whole xunit file in ocs-ci as we can then just use it even in some manual upload to polarion without any further changes, and in the future we will need maybe add more properties.

@clacroix12
Copy link
Contributor Author

I wanted to bring attention to a small issue I'm seeing with these updates. In adding the log statement in ocscilib.py it seems like some of the pytest output gets captured and placed in the <system-out> of one of my test functions in the junit report xml. I'm not exactly sure why and it appears to only happen for the first test case that is executed. I have confirmed that simply commenting out the log statement is enough to get this output to not show up in the junit xml.

    <testcase classname="tests.test_pytest_html.TestPolarionExample"
              file="tests/test_pytest_html.py" line="30"
              name="test_without_marker" time="0.003">
        <system-out>#x1B[1m
            collected 3 items #x1B[0m

            tests/test_pytest_html.py::TestPolarionExample::test_without_marker
        </system-out>
    </testcase>

This isn't a blocker and as such didn't want to hold up this PR for it, but I wanted to call it out and see if anyone would have any ideas as to why this is happening.

vasukulkarni
vasukulkarni previously approved these changes Jun 18, 2019
mbukatov
mbukatov previously approved these changes Jun 19, 2019
Copy link
Contributor

@mbukatov mbukatov left a comment

Choose a reason for hiding this comment

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

Looks good. The only problem is in the default value of project_id, but this could be reconfigured.

conf/ocsci/default_config.yaml Outdated Show resolved Hide resolved
ocs/defaults.py Outdated Show resolved Hide resolved
@mbukatov
Copy link
Contributor

This isn't a blocker and as such didn't want to hold up this PR for it, but I wanted to call it out and see if anyone would have any ideas as to why this is happening.

This looks like a peculiarity of the pytest xml report. Logs and output from collecting phase doesn't belong to any test and so the xml report seems to just add it into the 1st one instead of dropping it from the xml report.

I don't think this is a big deal, as in normal operation in CI or any other valid test run, we are not expected to have any warnings reported in pytest hooks. The warning here is for author of the test code to notice something needs to be addressed before merging.

@clacroix12 clacroix12 dismissed stale reviews from mbukatov and vasukulkarni via ac00be1 June 19, 2019 15:09
mbukatov
mbukatov previously approved these changes Jun 19, 2019
Signed-off-by: Coady LaCroix <clacroix@redhat.com>
Signed-off-by: Coady LaCroix <clacroix@redhat.com>
Signed-off-by: Coady LaCroix <clacroix@redhat.com>
…without providing a value

Signed-off-by: Coady LaCroix <clacroix@redhat.com>
Signed-off-by: Coady LaCroix <clacroix@redhat.com>
petr-balogh
petr-balogh previously approved these changes Jun 19, 2019
Signed-off-by: Coady LaCroix <clacroix@redhat.com>
@clacroix12 clacroix12 merged commit bd1e912 into red-hat-storage:master Jun 19, 2019
@clacroix12 clacroix12 deleted the polarion_support branch June 19, 2019 19:14
@RazTamir
Copy link
Contributor

With this PR is now merged, could you please provide information, what is needed for test cases to include in order to be in sync with Polarion?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request High Priority High priority issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants