Create a new module: new_test_util #111
Conversation
…st_util.py. It includes a number of changes: * Uses junit-xml module to create test xml files instead of manually creating them * Show test case logs in gubernator (so that we don't have to rely on argo to show logs) * Reduce boilerplate code required to create a new test * Wrap every test case so that one failing test case does not prevent other tests from running * Provide an init method which handles heavy lifting of setting up logging and common args Also * Refactor py_checks into test_py_checks and test_py_lint * These are written using new_test_util module * Eventually the plan is to move all existing tests across Kubeflow to new_test_util * Move from mlkube-testing to kubeflow-ci for test worker image
ankushagarwal
changed the title
py/kubeflow/testing/new_test_util.py
Outdated
| self.test_func = test_func | ||
| super(TestCase, self).__init__(**kwargs) | ||
|
|
||
| def add_failure_info(self, message=None, output=None, failure_type=None): |
There was a problem hiding this comment.
please add doc. what's this function for and what is each arg.
py/kubeflow/testing/new_test_util.py
Outdated
|
|
||
| def generate_xml(self): | ||
| output_file = os.path.join(self.artifacts_dir, | ||
| "junit_" + self.name + ".xml") |
There was a problem hiding this comment.
I remember we should replace "_" by "-" (or the opposite) in the name?
There was a problem hiding this comment.
I tested _ and it seems to work.
The Gubernator example https://github.com/kubernetes/test-infra/tree/master/gubernator also uses _
There was a problem hiding this comment.
See #638.
https://github.com/kubeflow/kubeflow/pull/638/files#diff-2552a6a0cb5a74e3184790464405bedcR309
test-grid is the problem and we want only 1 "_" in the name.
Please fix and a comment explaining why we do this.
py/kubeflow/testing/new_test_util.py
Outdated
| def generate_xml(self): | ||
| output_file = os.path.join(self.artifacts_dir, | ||
| "junit_" + self.name + ".xml") | ||
| ET.ElementTree(ET.fromstring(junit_xml.TestSuite.to_xml_string( |
There was a problem hiding this comment.
what's this elementTree doing?
There was a problem hiding this comment.
My bad. This long chained function call is not easy to understand. Added comment to explain what's happening and broke this down into 3 separate calls.
|
/retest |
py/kubeflow/testing/new_test_util.py
Outdated
|
|
||
| This is intended to replace test_util.py | ||
| """ | ||
| import argparse |
There was a problem hiding this comment.
Could we just call this package util.py?
The package is kubeflow.testing
so unless we already have a util.py file that seems fine. And this avoids the prefix "new".
We should avoid the prefix "new" because at some point we'd probably want to rename it anyway since the "new" will no longer be relevant and then we'd just have to update a bunch of call sites.
So it would be better to pick a name that we won't want to change.
There was a problem hiding this comment.
We already have a util.py
There was a problem hiding this comment.
Renamed to test_helper.py
|
nice change. |
|
/lgtm but I only did a drive by review to respond to the issue with "_" so please wait for Lunkai to review. |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jlewi, lluunn The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/hold cancel |
* Update py_lint and py_test kubeflow/testing#111 refactored these two tests into two separate files /cc @gaocegege * Ignore jupyterhub_spawner.py
* Update py_lint and py_test kubeflow/testing#111 refactored these two tests into two separate files /cc @gaocegege * Ignore jupyterhub_spawner.py
* Update py_lint and py_test kubeflow/testing#111 refactored these two tests into two separate files /cc @gaocegege * Ignore jupyterhub_spawner.py
Create a new module: new_test_util.py. This is intended to replace test_util.py. It includes a number of changes:
Also
Refactor py_checks into test_py_checks and test_py_lint
Eventually the plan is to move all existing tests across Kubeflow to new_test_util
Move from mlkube-testing to kubeflow-ci for test worker image
/cc @lluunn
/cc @jlewi