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

Add UniqueID to XML Output #5092

Merged
merged 4 commits into from
Sep 1, 2021
Merged

Conversation

theishshah
Copy link
Member

Signed-off-by: Ish Shah ishah@redhat.com

Include optional Unique ID in XML output, change was withheld earlier as an API update was necessary

Signed-off-by: Ish Shah <ishah@redhat.com>
@estroz
Copy link
Member

estroz commented Jul 28, 2021

@theishshah can you link the api repo PR, and add a changelog entry here

@theishshah
Copy link
Member Author

operator-framework/api#110

Here is the API PR, this one merged a while back. The type and name were chosen based on QE's requests for it in Xunit.

Signed-off-by: Ish Shah <ishah@redhat.com>
@@ -142,8 +142,7 @@ func (c *scorecardCmd) convertXunit(output v1alpha3.TestList) xunit.TestSuites {
}
tSuite.TestCases = append(tSuite.TestCases, tCase)
tSuite.URL = item.Spec.Image
//TODO: Add TestStuite ID when API updates version
//tSuite.ID = item.Spec.UniqueID
tSuite.ID = item.Spec.UniqueID
Copy link
Member

Choose a reason for hiding this comment

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

What happens if item.Spec.UnqiueID is unset? Does tSuite.ID stay empty?

Copy link
Member Author

Choose a reason for hiding this comment

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

Just ran it with UniqueID unset to confirm, tSuite.ID stays empty.

Copy link
Member

Choose a reason for hiding this comment

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

Well this should probably have a default (random) value if not configured with one right?

Copy link
Member Author

Choose a reason for hiding this comment

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

So the only users of this field are folks using this for xunit, and in those scenarios the ID's are being fed in the config anyways and there aren't really other use cases for this value (since the testcase names themselves are also unique).

Copy link
Member

Choose a reason for hiding this comment

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

Will leaving the ID field of XUnit output empty cause errors when an XUnit parser attempts to parse this stuff? If so we should either set this to the testcase name, some random string, or (probably better) error out.

Copy link
Member Author

Choose a reason for hiding this comment

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

That's a good question I will find out. If it would cause errors, I agree with the solution of erroring out if the field is left empty. I am not opposed to setting the field to the testcase name if we absolute must populate it, as opposed to the current omit if empty behavior.

Copy link
Member Author

Choose a reason for hiding this comment

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

Having an empty UID field does not appear to cause issues in the polarion importer

Copy link
Member

@rashmigottipati rashmigottipati Aug 12, 2021

Choose a reason for hiding this comment

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

Is this an optional field that can be set or leave as unset?

Copy link
Member

Choose a reason for hiding this comment

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

Also, would this need to conform to any naming conventions?
Will the XUnit parser work for all special characters?

Copy link
Member

@rashmigottipati rashmigottipati left a comment

Choose a reason for hiding this comment

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

Have a couple of questions.

@@ -142,8 +142,7 @@ func (c *scorecardCmd) convertXunit(output v1alpha3.TestList) xunit.TestSuites {
}
tSuite.TestCases = append(tSuite.TestCases, tCase)
tSuite.URL = item.Spec.Image
//TODO: Add TestStuite ID when API updates version
//tSuite.ID = item.Spec.UniqueID
tSuite.ID = item.Spec.UniqueID
Copy link
Member

@rashmigottipati rashmigottipati Aug 12, 2021

Choose a reason for hiding this comment

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

Is this an optional field that can be set or leave as unset?

@@ -142,8 +142,7 @@ func (c *scorecardCmd) convertXunit(output v1alpha3.TestList) xunit.TestSuites {
}
tSuite.TestCases = append(tSuite.TestCases, tCase)
tSuite.URL = item.Spec.Image
//TODO: Add TestStuite ID when API updates version
//tSuite.ID = item.Spec.UniqueID
tSuite.ID = item.Spec.UniqueID
Copy link
Member

Choose a reason for hiding this comment

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

Also, would this need to conform to any naming conventions?
Will the XUnit parser work for all special characters?

@theishshah
Copy link
Member Author

Re: Rashmi's comments:

  1. Yes this is an optional field, and if omitted will be dropped
  2. Because this is an optional field, the assumption by the system to assume that the user input is clean, any naming conventions or symbols that aren't supported by the XUnit tools downstream shouldn't be fed in here

Signed-off-by: Ish Shah <ishah@redhat.com>
@rashmigottipati
Copy link
Member

Setting it to a default value if it is empty is a good idea.

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Aug 31, 2021
Signed-off-by: Ish Shah <ishah@redhat.com>
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Aug 31, 2021
Copy link
Member

@rashmigottipati rashmigottipati left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Aug 31, 2021
@theishshah theishshah merged commit 05d58e2 into operator-framework:master Sep 1, 2021
@theishshah theishshah deleted the uidXML branch September 1, 2021 15:41
@theishshah theishshah restored the uidXML branch September 1, 2021 15:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants