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

Check that test case attribute exists in junit xml file before converting it #4623

Merged
merged 4 commits into from May 26, 2017

Conversation

Projects
None yet
3 participants
@dotordogh
Copy link
Contributor

dotordogh commented May 25, 2017

Problem

Issue 4619 was opened because an error is thrown when the time attribute is missing from a testcase in the junit xml file.

Solution

I added a check that will verify the time attribute has contents before trying to convert it to a float and added a test to ensure other missing attributes don't cause problems.

Result

With this change, users will not see parsing errors when running their tests. Fixes #4619.

{
'testOK': {
'result_code': 'success',
'time': ''

This comment has been minimized.

@baroquebobcat

baroquebobcat May 25, 2017

Contributor

I think it'd be better to either leave all the times as strings, or use None. It bothers me a bit to mix data types since '' may mean something different from null.

This comment has been minimized.

@stuhood

stuhood May 25, 2017

Member

IMO, floating point seconds or "unset" would be fine... but null seems unnecessary.

This comment has been minimized.

@baroquebobcat

baroquebobcat May 26, 2017

Contributor

I'd prefer None, since I think it is clearer than '' or 'unset', and some languages, like Python, allow strings to be manipulated by numeric operators, like *, which can cause weird bugs.

This comment has been minimized.

@stuhood

stuhood May 26, 2017

Member

Er, I'm not suggesting the literal string 'unset'... I'm suggesting that just not putting it in the dict might be preferable. But None is fine, so let's leave it at that.

This comment has been minimized.

@dotordogh

dotordogh May 26, 2017

Contributor

@stuhood @baroquebobcat I can remove the time all together if you'd like. Whatever you this is best.

@baroquebobcat
Copy link
Contributor

baroquebobcat left a comment

lgtm, aside from a small additional doc request.

try:
test_info.update({'time': float(testcase.getAttribute('time'))})
except:
test_info.update({'time': None})

This comment has been minimized.

@baroquebobcat

baroquebobcat May 26, 2017

Contributor

Could you also note that time may be None in the parse_test_info docstring?

{
'testOK': {
'result_code': 'success',
'time': ''

This comment has been minimized.

@baroquebobcat

baroquebobcat May 26, 2017

Contributor

I'd prefer None, since I think it is clearer than '' or 'unset', and some languages, like Python, allow strings to be manipulated by numeric operators, like *, which can cause weird bugs.

@stuhood
Copy link
Member

stuhood left a comment

Thanks Dorothy!

@stuhood stuhood added this to the 1.3.0 milestone May 26, 2017

@stuhood stuhood merged commit 9ca3732 into pantsbuild:master May 26, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

stuhood added a commit that referenced this pull request May 26, 2017

Check that test case attribute exists in junit xml file before conver…
…ting it (#4623)

### Problem

[Issue 4619](#4619) was opened because an error is thrown when the time attribute is missing from a testcase in the junit xml file.

### Solution

I added a check that will verify the time attribute has contents before trying to convert it to a float and added a test to ensure other missing attributes don't cause problems. 

### Result

With this change, users will not see parsing errors when running their tests. Fixes #4619.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment