weixin_39671509
weixin_39671509
2021-01-11 04:24

Collecting launcher

Overview

Add CollectingLauncher which provides access to TestCollection.

The TestCollection allows users of Launcher to review the test plan, do additional filtering, and execute the tests, without going through a potentially expensive discovery stage more than once.

This pull contains three commits. Please don't squash them.

I hereby agree to the terms of the JUnit Contributor License Agreement.

Definition of Done

该提问来源于开源项目:junit-team/junit5

  • 点赞
  • 写回答
  • 关注问题
  • 收藏
  • 复制链接分享
  • 邀请回答

12条回答

  • weixin_39671509 weixin_39671509 4月前

    it's my understanding that if I walk up the TestDescriptor tree (by repeatedly calling getParent()) it will stop at a root node. If there are multiple test engines, there could be multiple root nodes. So if I go with your proposal, it sounds like my TestDescriptor would have to build a shard map based on an incomplete view of the tests included in the TestPlan, and later update the shard map as JUnit calls repeatedly calls my filter function. That would be a lot more complicated than traversing the TestPlan. It's also wouldn't be deterministic unless JUnit provides some guarantees about ordering of calls to the filter function.

    It would be much simpler if I could examine all of the leaf nodes in the test plan before the start of the post-discovery-filter phase. JUnit 4 supports this (by having a root Runner that allows me to traverse the Description tree independently of running the tests). I'm just proposing a way to support the same thing in JUnit5.

    点赞 评论 复制链接分享
  • weixin_39991222 weixin_39991222 4月前

    I'm hesitant to add another concept to the Launcher interface.

    How about we add a initialize(Map<String, TestDescriptor> engineTestDescriptorById) method to PostDiscoveryFilter? That would give it a chance to prepare the shard map and apply it in subsequent calls to apply(TestDescriptor).

    If the shard map should be garbage collected or other resources need to be cleared, we could add another cleanUp() method or similar. Both could have empty implementations by default.

    点赞 评论 复制链接分享
  • weixin_39671509 weixin_39671509 4月前

    I assume by "another concept" you mean TestCollection? Although I understand the desire to not have one-off interfaces, here I am extracting out an API for a concept that already exists in the code: the collection of tests that is being filtered during the discovery process. If you look at the third commit, this interface comes in useful for the JUnitPlatform where the filtering code now reuses the classes used by the Launcher implementation.

    Your idea of an initialize method in PostDiscoveryFilter is an interesting option, and should work for Bazel. Unfortunately, I don't expect to have much time for JUnit5 in 2018. If after my response above you still think that updating PostDiscoveryFilter is your preferred option, someone else will likely have to implement it.

    点赞 评论 复制链接分享
  • weixin_39991222 weixin_39991222 4月前

    Maybe it isn't "another concept", maybe it's just the wording that puts me off. I'm not excited about TestCollection collect(...). It actually does perform discovery, so I'd like to keep that wording. Maybe PreliminaryDiscoveryResult discoverPreliminarily(...) or FilterableDiscoveryResult discoverForFiltering(...) or something along those lines. 🤔

    Moreover, I'd like to get rid of the separate interface. Launcher isn't really intended to be implemented by clients. I think we could add the additional method with a default implementation that throws an exception to formally stay binary compatible.

    点赞 评论 复制链接分享
  • weixin_39991222 weixin_39991222 4月前

    We're cleaning out the issue tracker and closing PRs for issues that we've not seen much demand to fix. Feel free to comment with additional justifications if you feel that this one should not have been closed.

    点赞 评论 复制链接分享
  • weixin_39671509 weixin_39671509 4月前

    Friendly ping

    点赞 评论 复制链接分享
  • weixin_39991222 weixin_39991222 4月前

    Looks interesting for sure! Can you outline how this would be used by tools like Bazel?

    I'm wondering if we could store Root and ConfigurationParameters in TestPlan and get away without introducing another method in Launcher/CollectionLauncher.

    点赞 评论 复制链接分享
  • weixin_39671509 weixin_39671509 4月前

    Can you outline how this would be used by tools like Bazel?

    For sharded tests, Bazel passes the number of shards and the current shard as environment variables. So if there are 3 shards, the test will run 3 times in parallel in different processes, each time with NUM_SHARDS=3 but each with a different value of CURRENT_SHARD.

    Bazel's JUnit test runner would:

    1. Call CollectingLauncher.collect() to get a TestCollection
    2. Call TestCollection.testPlan()
    3. Iterate over the leaf nodes and create a sorted list of tests (this is for correctness in determining the shard map, in case the ordering is different between test runs; Bazel won't run the tests in this order)
    4. Assign each test in that list to a shard by iterating through the list and assigning the shard to index % numShards(producing a Map<TestIdentifier, Index> that maps a test to a shard)
    5. Call applyPostDiscoveryFilters() to filter out tests that are not on the "current" shard
    6. Call execute() to run the tests

    Without this change. Bazel would have to go through the discovery phase twice. For large projects with many classes and jars, scanning classes on the classpath can take several seconds.

    I'm wondering if we could store Root and ConfigurationParameters in TestPlan and get away without introducing another method in Launcher/CollectionLauncher

    That would only help if there was a way to filter and execute a TestPlan (which is what TestCollection does in my pull).

    点赞 评论 复制链接分享
  • weixin_39671509 weixin_39671509 4月前

    Friendly ping

    点赞 评论 复制链接分享
  • weixin_39991222 weixin_39991222 4月前

    Thanks for the ping! I'll take a look on the weekend.

    点赞 评论 复制链接分享
  • weixin_39991222 weixin_39991222 4月前

    Bazel's JUnit test runner would:

    1. Call CollectingLauncher.collect() to get a TestCollection
    2. Call TestCollection.testPlan()
    3. Iterate over the leaf nodes and create a sorted list of tests (this is for correctness in determining the shard map, in case the ordering is different between test runs; Bazel won't run the tests in this order)
    4. Assign each test in that list to a shard by iterating through the list and assigning the shard to index % numShards(producing a Map<TestIdentifier, Index> that maps a test to a shard)
    5. Call applyPostDiscoveryFilters() to filter out tests that are not on the "current" shard
    6. Call execute() to run the tests

    If I understood you correctly, Bazel will still have to go through this process for each shard, right?

    Wouldn't a PostDiscoveryFilter be able to assign the shard index on the fly and filter at the same time?

    点赞 评论 复制链接分享
  • weixin_39991222 weixin_39991222 4月前

    Well, when a PostDiscoveryFilter is called, it gets a reference to a TestDescriptor which provides access to the test tree for the entire engine it was created by. So it could build a shard map for each engine. Would that be feasible?

    点赞 评论 复制链接分享

相关推荐