Limit the amount of value reflection data expectation checking collects by default #915
Conversation
…ts by default Fixes rdar://138208832
stmontgomery
added
enhancement
stmontgomery
requested review from
grynspan,
briancroom,
SeanROlszewski and
suzannaratcliff
as code owners
| /// maximum count. After this maximum is reached, all subsequent elements are | ||
| /// omitted and a single placeholder child is added indicating the number of | ||
| /// elements which have been truncated. | ||
| public var maximumValueReflectionCollectionCount: Int = 10 |
There was a problem hiding this comment.
| public var maximumValueReflectionCollectionCount: Int = 10 | |
| public var maximumValueReflectionCollectionCount = 10 |
There was a problem hiding this comment.
I like having the types of properties with public or greater access level state their type explicitly, to avoid the possibility of the inferred type changing without our knowledge and causing a API breakage. I see now that we have not done this with some trivial properties in Configuration, but now that I've moved most of these into a nested struct I've used this style within that new struct at least. I'd like to apply this coding style more broadly in a subsequent PR.
There was a problem hiding this comment.
We pretty much never do this for variables to which we assign values. We only specify types on variables that will be assigned values later. Consistency would be good (either way.)
There was a problem hiding this comment.
Already merged, but I did strip off the explicit types from these properties for now, for consistency!
|
Also please keep #840 in the back of your mind as you make changes here. |
|
@swift-ci please test |
|
@swift-ci please test |
…#926) This amends the code style guidelines for the project to require an explicit type for all properties whose access level is `public` or greater, and adjusts the code accordingly. ### Motivation: See the explanation in the code style document for rationale. This topic recently [came up](#915 (comment)) in a PR discussion. ### Checklist: - [x] Code and documentation should follow the style of the [Style Guide](https://github.com/apple/swift-testing/blob/main/Documentation/StyleGuide.md). - [x] If public symbols are renamed or modified, DocC references should be updated.
This modifies the internal logic which recursively collects reflection data about values passed to
#expect(...)and similar expectation APIs so that it imposes an artificial limit on the amount of data collected.Motivation:
Some users have attempted to use
#expectwith instances of values with large collections or with deep value hierarchies involving many sub-properties, etc. In these situations they have noticed that the automatic value reflection is adding a noticeable amount of overhead to the overall test execution time, and would like to reduce that by default (near term), as well as have some ability to control that so the behavior could be tweaked for particular tests (in the future).Modifications:
Configurationproperties controlling data collection behaviors and setting default limits.Expression.Value.Expression.Value.init(reflecting:)optional and returnnilwhen value reflection is disabled in the configuration.Expression.Value.init(describing:)as a lighter-weight alternative toinit(reflecting:)which only forms a string description of its subject, and adopt this as a fallback in places affected by the new optional in the previous bullet.Checklist:
Fixes rdar://138208832