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

JUnit Test Report file parsing fails when a test case is missing the "time" attribute #4619

Closed
PhocianFirewall opened this Issue May 24, 2017 · 3 comments

Comments

Projects
None yet
3 participants
@PhocianFirewall
Copy link

PhocianFirewall commented May 24, 2017

Following the upgrade to 1.3.0rc2, we started receiving multiple errors while pants is trying to process the JUnit test result files. The errors are of form:

Error parsing test result file /path/to/TEST-suite.xml: could not convert string to float

This is a regression from the previous version, 1.3.0.dev19. Upon investigating, it became apparent that every test suite with an error has at least one test case that lacks the "time" attribute in the test report. For example, TEST-mypackage.MyTest.xml might have the following contents:

   ...
    <testcase classname="mypackage.MyTest" name="test1"/>
    <testcase classname="mypackage.MyTest" name="test2" time="0.001"/>
    <testcase classname="mypackage.MyTest" name="test3" time="0.004"/>
   ...

The error appears to be caused by the following commit: https://github.com/pantsbuild/pants/pull/4561/files. The problematic code appears to be on line 120 in src/python/pants/task/testrunner_task_mixin.py:

test_info = {'time': float(testcase.getAttribute('time'))}

The code doesn't check if the "time" attribute before converting it to a float.

@baroquebobcat

This comment has been minimized.

Copy link
Contributor

baroquebobcat commented May 24, 2017

Thanks for the report!

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

@stuhood

This comment has been minimized.

Copy link
Member

stuhood commented May 24, 2017

Thanks! Marked as a 1.3.0 blocker. rc2 and rc3 ran our entire internal sandbox and saw this in only one case (which succeeded after retry), so it's interesting that you're seeing it more frequently.

@PhocianFirewall

This comment has been minimized.

Copy link

PhocianFirewall commented May 24, 2017

@stuhood Thanks for the quick reply! I didn't dig in too deeply into when the "time" attribute is omitted from test reports, but we have a ton of those cases in our code base, especially for ScalaTest tests. In particular, I've noticed that every time a test is canceled (e.g. because an assume condition doesn't hold up), the "time" attribute is omitted.

stuhood added a commit that referenced this issue 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.

stuhood added a commit that referenced this issue 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