Allow to opt out of requesting TestRun details and turn off defaultFilter #80
Conversation
IvanPashchenko
commented
Mar 17, 2020
IvanPashchenko
commented
- The details of the test run can be extremely large and are not always needed, so now it's possible to opt out of requesting them from server
- The defaultFilter hides some builds and there was no way to find them previously
…an skip some (e.g. details, which can be _very_ big)
| @@ -187,6 +189,20 @@ interface TestRunsLocator { | |||
| fun forProject(projectId: ProjectId): TestRunsLocator | |||
| fun withStatus(testStatus: TestStatus): TestRunsLocator | |||
| fun all(): Sequence<TestRun> | |||
| fun all(fields: EnumSet<TestRunFields>): Sequence<TestRun> | |||
There was a problem hiding this comment.
Without KDoc it isn't evident that this function does. Also this interface uses builder style to avoid adding overloads. So it'll be more consistent and convenient to add a separate function (limitFields?) instead. And in this case the function name may be self-documenting so we won't need to add KDoc.
| fun all(fields: EnumSet<TestRunFields>): Sequence<TestRun> | ||
| } | ||
|
|
||
| enum class TestRunFields { |
There was a problem hiding this comment.
Are you sure we really need to allow excluding every possible field here? You said that "The details of the test run can be extremely large" so maybe it's enough to skip details and ignoreDetails fields? In that case it'll be enough to add a function withoutDetails to TestRunsLocator, this will greatly simplify API and implementation. Also note that currently if someone exclude Build, Name or Test field, and then try to access corresponding property in TestRun, he'll get an exception, it may be unexpected.
Note that providing access to all possible options in TeamCity REST API is not a goal for this library. It should provide convenient API for real use cases.
| @@ -144,6 +144,8 @@ interface BuildLocator { | |||
| */ | |||
| fun withAllBranches(): BuildLocator | |||
|
|
|||
| fun withDefaultFilter(enabled: Boolean): BuildLocator | |||
There was a problem hiding this comment.
Meaning of this function is unclear, I needed to look into TeamCity REST API doc to understand what it is about. One of the goals of this library is to provided clear API which can be used without having to look into REST API doc (see CONTRIBUTING.md).
Well, we can put text from the documentation to KDoc here, but it doesn't help much. When users need to invoke this function? You wrote "The defaultFilter hides some builds and there was no way to find them previously". Could you please provide more details which builds weren't included and which options in BuildLocator did you use? E.g. if in order to show cancelled or personal builds we need to set defaultFilter to false, we may do this automatically in BuildLocatorImpl::all without adding this function to API.
There was a problem hiding this comment.
Well I don't really know how to describe what default filter does as the logic behind it can change, but the point is that it hides the builds from user even if you made all possible calls in the builder, so the option is very much needed.
Regarding adding it by default - I'm all for that, but that changes the previous behavior and after the update, users may be surprised as to why do they get more builds.
To reiterate: I'll gladly make it turned off by default. Do you think that we need a method like withDefaultFilter() which will just turn it on?
There was a problem hiding this comment.
val dependencies = teamCityInstance.builds()
.includeFailed()
.includeCanceled()
.withAllBranches()
.snapshotDependencyTo(build.id)
.all()
.toList()
this is what I used, and IIRC the builds that failed to start were not included
IvanPashchenko
changed the title
# Conflicts: # gradle.properties # teamcity-rest-client-api/src/main/kotlin/org/jetbrains/teamcity/rest/api.kt # teamcity-rest-client-impl/src/main/kotlin/org/jetbrains/teamcity/rest/implementation.kt # teamcity-rest-client-impl/src/main/kotlin/org/jetbrains/teamcity/rest/rest.kt
