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

Report JUnit test results for all TVM Python tests #7450

Merged
merged 5 commits into from Feb 17, 2021

Conversation

areusch
Copy link
Contributor

@areusch areusch commented Feb 12, 2021

This change enables JUnitXML output from pytest for all TVM python testing. The goal is to enable us to better understand which TVM tests are slow, how a change might impact them, and the runtime variance we see in the TVM CI.

See the example test report produced from the staging run on this PR.

Note that a previous staging run did timeout in the frontend tests, but by a long shot, so I think there's a chance it was unrelated to this PR.

For discussion

  • The JUnit plugin supports longitudinal test time tracking (click the right arrow > button on the graph) at both the test level and at the test group level.
  • Right now, some tests are represented twice: once for cython and once for ctypes. Because these don't appear in the test class path, test time at the group level can lump in both. pytest can be configured to add a prefix per-run, so we could fix this easily right now. On the other hand, adding the prefix could clutter the test output and make it harder to scan.
  • Do you prefer to prefix the test case names e.g. cython-tests.python.unittest.test_arith_canonical_simplify.test_split_index_simplify?

@tqchen @junrushao1994 @tkonolige @jroesch @zhiics @comaniac @u99127 @manupa-arm @leandron

Copy link
Contributor

@leandron leandron left a comment

Choose a reason for hiding this comment

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

Do you prefer to prefix the test case names e.g. cython-tests.python.unittest.test_arith_canonical_simplify.test_split_index_simplify?

I think the prefixing is a good idea, to make that obvious. I never got into the internals of JUnit reporting (just used directly to publish on Jenkins, as you're enabling here) so I don't know how complex that change is.

Out of curiosity, are the test scripts already publishing the xml reports?

@areusch
Copy link
Contributor Author

areusch commented Feb 12, 2021

@leandron yeah that was enabled in #7407

Copy link
Contributor

@tkonolige tkonolige left a comment

Choose a reason for hiding this comment

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

Prefixing sounds good to me.

Copy link
Contributor

@u99127 u99127 left a comment

Choose a reason for hiding this comment

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

LGTM.

@areusch
Copy link
Contributor Author

areusch commented Feb 16, 2021

I had thought I pushed 6a77cdd to ci-docker-staging on friday, but looks like not. let's do that now and wait to merge til we see the result

@areusch
Copy link
Contributor Author

areusch commented Feb 17, 2021

ok this is ready to merge. I tested with the prefix option, the output is here.

I made one final change (remove the - in the prefix to make it look like a python namespace). I don't think we need to run the one-character change in 205871e through staging CI, but we can if that's policy.

@areusch
Copy link
Contributor Author

areusch commented Feb 17, 2021

@leandron please explicitly approve if you're ok w/ this.

Copy link
Contributor

@leandron leandron left a comment

Choose a reason for hiding this comment

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

Yes, LGTM - thanks @areusch

@junrushao junrushao merged commit fe398bf into apache:main Feb 17, 2021
Lokiiiiii pushed a commit to Lokiiiiii/tvm that referenced this pull request Mar 2, 2021
* Enable JUnit parsing for Python tests

* retrigger CI

* prefix junit results with FFI type

* remove - in junit prefix
trevor-m pushed a commit to neo-ai/tvm that referenced this pull request Mar 2, 2021
* Enable JUnit parsing for Python tests

* retrigger CI

* prefix junit results with FFI type

* remove - in junit prefix
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.

None yet

5 participants