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

Better support --test-junit-test with classname, remove support for file path #7589

Merged
merged 16 commits into from Apr 23, 2019

Conversation

Projects
None yet
3 participants
@wisechengyi
Copy link
Contributor

commented Apr 18, 2019

New behavior

    Force running of just these tests. Tests can be specified using any of:
    [classname], [classname]#[methodname], [fully qualified classname], [fully
    qualified classname]#[methodname]. If classname is not fully qualified, all
    matching tests will be run. For example, if `foo.bar.TestClass` and
    `foo.baz.TestClass` exist and `TestClass` is supplied, then both will run.

Sample Result

$ ./pants --cache-test-junit-ignore test examples/tests/java/org/pantsbuild/example/hello/greet/ examples/tests/java/org/pantsbuild/example/usewire: --test-junit-test=GreetingTest#mentionGreetee 
...
                   Invalidated 2 targets.
18:49:23 00:03       [run]
                     Auto-detected 8 processors, using -parallel-threads=8
                     ..
                     Time: 0.038
                     
                     OK (1 test)
                     
                     
                   examples/tests/java/org/pantsbuild/example/hello/greet                          .....   SUCCESS
                   examples/tests/java/org/pantsbuild/example/usewire                              .....   SUCCESS

@wisechengyi wisechengyi requested review from stuhood and benjyw Apr 18, 2019

@jsirois
Copy link
Member

left a comment

The code tries awfully hard to support all four:
https://github.com/wisechengyi/pants/blob/cd0abeb6b6f1526fb2f4f72c83961af2decfc463/src/python/pants/backend/jvm/tasks/junit_run.py#L46-L76

Do you mind cleaning up dead code to match your help change or else fixing the code and reverting your help change?

@stuhood

This comment has been minimized.

Copy link
Member

commented Apr 18, 2019

This is possibly related to #6239 =/

@wisechengyi

This comment has been minimized.

Copy link
Contributor Author

commented Apr 18, 2019

will try to fix. if it's too expensive, will just remove the non working part.

@wisechengyi wisechengyi changed the title Update Junit runner help message [WIP] Better support --test-junit-test with classname, remove support for file name Apr 19, 2019

@wisechengyi

This comment has been minimized.

Copy link
Contributor Author

commented Apr 19, 2019

Still WIP. Going to write/modify some tests.

wisechengyi added some commits Apr 19, 2019

@stuhood

This comment has been minimized.

Copy link
Member

commented Apr 20, 2019

I talked with @wisechengyi about this offline, and I think that removing this support is the right choice. zinc provides the only realistic way I know of to discover "classes-per-source" for scala, and I don't think that we're married to zinc.

wisechengyi added some commits Apr 20, 2019

@wisechengyi wisechengyi changed the title [WIP] Better support --test-junit-test with classname, remove support for file name [WIP] Better support --test-junit-test with classname, remove support for file name/path Apr 21, 2019

@wisechengyi wisechengyi changed the title [WIP] Better support --test-junit-test with classname, remove support for file name/path Better support --test-junit-test with classname, remove support for file path Apr 21, 2019

@wisechengyi

This comment has been minimized.

Copy link
Contributor Author

commented Apr 21, 2019

ready for review

Tests can be specified in one of four forms:
* [classname]

This comment has been minimized.

Copy link
@jsirois

jsirois Apr 21, 2019

Member

Just two forms:

  • [fully qualified classname
  • [fully qualified classname]#[methodname]

In order to support [classname] variants of this we'd have to support some sort of policy for finding unqualified classnames which I'm pretty sure we'd all agree we don't want to wade into... but you did below! I vote to stick to the PR purpose and just trim the option help / code to do what it claims and not add new functionality (guessing unqualifed class names). That is better added in a follow up iff there is agreement this is a wise feature to add.

@wisechengyi

This comment has been minimized.

Copy link
Contributor Author

commented Apr 21, 2019

msg
for test_spec in self._test_to_target.keys():
fqcn = test_spec.classname
if fqcn in fqcn_to_specs:
raise TaskError('Dup found: {}'.format(fqcn))

This comment has been minimized.

Copy link
@jsirois

jsirois Apr 22, 2019

Member

It looks like this fails a command line of ./pants test.junit --test=org.example.MyTestClass#testMethod1 --test=org.example.MyTestClass#testMethod2. Do I have that right? If so, this seems like it should be allowed. As I read, it was allowed previously so this is a breaking change.

This comment has been minimized.

Copy link
@wisechengyi

wisechengyi Apr 22, 2019

Author Contributor

This is more of a sanity check to make sure what Pants knows (self._test_to_target) are all unique fully qualified names.

The example still works.

$ ./pants --cache-test-junit-ignore test examples/tests/java/org/pantsbuild/example/hello/:: \
--test-junit-test=org.pantsbuild.example.hello.greet.GreetingTest#mentionGreetee \
--test-junit-test=org.pantsbuild.example.hello.greet.GreetingTest#shouldSayHello
...
10:58:51 00:03     [test-jvm-prep-command]
10:58:51 00:03       [jvm_prep_command]
10:58:51 00:03     [test-prep-command]
10:58:51 00:03     [test]
10:58:51 00:03     [pytest-prep]
10:58:51 00:03     [pytest]
10:58:51 00:03     [junit]
                   Invalidated 2 targets.
10:58:51 00:03       [run]
                     Auto-detected 8 processors, using -parallel-threads=8
                     ....
                     Time: 0.045
                     
                     OK (2 tests)
                     
                     
                   examples/tests/java/org/pantsbuild/example/hello/greet                          .....   SUCCESS
                   examples/tests/java/org/pantsbuild/example/hello/xyz                            .....   SUCCESS
10:58:52 00:04     [go]
10:58:52 00:04     [node]
               Waiting for background workers to finish.
10:58:52 00:04   [complete]
               SUCCESS

This comment has been minimized.

Copy link
@wisechengyi

wisechengyi Apr 22, 2019

Author Contributor

Plan to merge this by EOD. @jsirois please let me know if there are additional concerns.

This comment has been minimized.

Copy link
@jsirois

jsirois Apr 22, 2019

Member

It does appear to work. That said, I hope you find this as distrubing as I do:

test_registry = RegistryOfTests(tuple(self._calculate_tests_from_targets(targets)))
if targets and self._tests_to_run:
matched_spec_to_target, unknown_tests = test_registry.match_test_spec(
self._get_possible_tests_to_run())
if len(unknown_tests) > 0:
raise TaskError("No target found for test specifier(s):\n\n '{}'\n\nPlease change "
"specifier or bring in the proper target(s)."
.format("'\n '".join(t.render_test_spec() for t in unknown_tests)))
return RegistryOfTests(matched_spec_to_target)

The RegistryOfTests constructed on line 254 would fail in the way I described if it had match_test_spec called on it. In other words this works as things happen to be used, but just looking at RegistryOfTests itself in isolation - it can be validly constructed with {Test('f.q.c.n', 'method1'): ..., Test('f.q.c.n', 'method2'): ...} - in which case it would be wonderful not to blow up here.

I still contend the assertion is not necessary. I believe this change keeps your current functionality while allowing for a RegistryOfTests initially constructed with >1 Test of the same classname:

diff --git a/src/python/pants/java/junit/junit_xml_parser.py b/src/python/pants/java/junit/junit_xml_parser.py
index 022d978c8..e9fbc662f 100644
--- a/src/python/pants/java/junit/junit_xml_parser.py
+++ b/src/python/pants/java/junit/junit_xml_parser.py
@@ -79,27 +79,19 @@ class RegistryOfTests(object):
     :return: dict test_spec -> target
     """
     # dict of non fully qualified classname to a list of fully qualified test specs
-    non_fqcn_to_specs = defaultdict(list)
-    fqcn_to_specs = {}
+    cn_to_specs = defaultdict(list)
     for test_spec in self._test_to_target.keys():
       fqcn = test_spec.classname
-      if fqcn in fqcn_to_specs:
-        raise TaskError('Dup found: {}'.format(fqcn))
-      fqcn_to_specs[fqcn] = test_spec
+      cn_to_specs[test_spec.classname].append(test_spec)
 
       non_fqcn = fqcn.split('.')[-1]
-      non_fqcn_to_specs[non_fqcn].append(test_spec)
+      cn_to_specs[non_fqcn].append(test_spec)
 
     matched_spec_to_target = {}
     unknown_tests = []
     for possible_test_spec in possible_test_specs:
-      # Match for fully qualified test spec
-      if possible_test_spec.classname in fqcn_to_specs:
-        matched_spec_to_target[possible_test_spec] = self._test_to_target[
-          fqcn_to_specs[possible_test_spec.classname]]
-      # Match for non fully qualified test specs
-      elif possible_test_spec.classname in non_fqcn_to_specs:
-        for full_spec in non_fqcn_to_specs[possible_test_spec.classname]:
+      if possible_test_spec.classname in cn_to_specs:
+        for full_spec in cn_to_specs[possible_test_spec.classname]:
           new_fully_qualified_test_spec = Test(full_spec.classname, possible_test_spec.methodname)
           matched_spec_to_target[new_fully_qualified_test_spec] = self._test_to_target[full_spec]
       else:

This comment has been minimized.

Copy link
@wisechengyi

wisechengyi Apr 22, 2019

Author Contributor

Nice I like the simplified code!

wisechengyi added some commits Apr 23, 2019

@wisechengyi wisechengyi merged commit 5cf8362 into pantsbuild:master Apr 23, 2019

1 check passed

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

@wisechengyi wisechengyi deleted the wisechengyi:junit-doc branch Apr 23, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.