feat(test): add shorthand property conformance tests

[kuzhelov]

Adds conformance tests for components' shorthand properties. These tests ensure that

• shorthand property is defined for component

• string value of shorthand property will be processed

<Button icon="chess rook" ... />

• object value of shorthand property will be spread to shorthand component as props

<Button icon={{ name: 'add-team', svg: true, ... }} ... />

---

Tests for icon property of Button component were added as well, to ensure this behavior and fix issues we've had before.

[codecov]

Codecov Report

Merging #112 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #112   +/-   ##
=======================================
  Coverage   88.37%   88.37%           
=======================================
  Files          45       45           
  Lines         757      757           
  Branches      109      109           
=======================================
  Hits          669      669           
  Misses         85       85           
  Partials        3        3

Impacted Files	Coverage Δ
src/components/Button/Button.tsx	93.93% <ø> (ø)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 62ac7b5...221524e. Read the comment docs.

[miroslavstastny]

_.lowerFirst()?

[kuzhelov]

actually, it turns out that this functionality is not needed, removed this altogether. Thanks!

[miroslavstastny]

Is this needed?
Can't you just call something like:

Icon.create(this.props.icon, {
  defaultProps: { /* xSpacing and others */ },
})}

Shorthand factory can handle string inherently.

[kuzhelov]

yes, lets do this way, thanks!

[levithomason]

Note, we only add entries for public facing changes. Chores, like tests, build, and CI changes do not get changelog entries as they do not affect end users.

[levithomason]

grammar nit: In the React community, props is not expanded to property. Suggest something like mapsValueToProp instead. This also follows the associated factory function name, mapValueToProps, another React community norm set by Redux.

[levithomason]

I don't think we should couple common tests with a chaining syntax like this. Inevitably, we are going to want to use a chained shorthand test without first passing a component to isConformant. Would prefer simple and explicit imports and separate calls here.

Propose we stick with the original test name of implementsShorthandProp. The base test is called isConformant as it tests many baseline standards ensuring the component conforms to them. We don't need to call every test "conformant" when dealing with isolated cases such as "shorthand prop", "className", "accessibility", etc. See previous prop vs property comment.

[kuzhelov]

let me, please, share my thoughts on that.

I don't think we should couple common tests with a chaining syntax like this. Inevitably, we are going to want to use a chained shorthand test without first passing a component to isConformant

actually, it is very hard for me to imagine this situation. Our library is about introducing only conformant components (as this is the main point of it - to ensure this consistency between all the components). Could you, please, suggest some hypothetical case where it might happen? Sorry, cannot come up with any by myself :(

Also, it seems that there are two aspects being mixed a bit. I am not against introducing it as a separate module, so that this functionality will be decoupled and modularized. At the same time, the question of the interface under which this functionality will be exposed to developer (chained calls, direct imports, etc) - this seems to be a separate question. The reason I might prefer chained syntax is that it is much more intuitive for the developer which testing methods she might consider (as well as that this chain, generally, describe the workflow for her):

• first, ensure that provided component is conformant
• then, optionally, ensure shorthand properties are properly handled
• ensure other aspects

And instead of looking on other specs for proper import statements, developer could rely on IntelliSense support:

[levithomason]

Responses

Examples of non-conformant components are Portal and Provider. Neither of these will pass the conformance test as they don't render typical UI components yet they still require other common tests. The Provider needs rendersChildren while the Portal needs hasSubcomponents and likely handlesAccessibility.

The list of components that will not pass isConformant will grow. In SUIR, we currently do not run conformance tests on Ref, TransitionalPortal, MountNode, or Transition yet all of these do import and run several other common tests.

Note that intellisense is still provided for all common tests when importing:

Proposal

Nonetheless, you raise a good point and perhaps we need to rethink what it means to be "conformant" if we expect many components to not conform.

In SUIR we were focused on making sure common UI components used by users conformed to many guidelines. Overtime, some components fell outside of this scope or had difficulty abstracting good tests around them. Stardust UI implemented conformance tests in largely the same way, but with some subtle differences and more capabilities given our *.info.json files for each component.

Maybe we trim down our conformance tests to strictly include tests that are applicable to all components. Things like naming and exports. Then add each test that does not apply to all components as chained calls? One upside is that all components can be handed to isConformant.

One downside to this is that it makes more room for error for most devs when creating most components (common UI components). It is nice to simply say isConformant and fix errors. However, having to know which chained calls they need to add could be daunting, especially if there are many.

Thoughts?

[levithomason]

I've left my feedback above. I will approve the PR now so as to not block you all day 👍 Please discuss these points with @miroslavstastny and I'll follow the decision you two agree to.

[kuzhelov]

agree with downsides of segregating tests, those will be especially critical now as we haven't critical mass of components for making a proper judgement. Lets follow the simplest path for now (the reason I wasn't sure about it before is that I wasn't aware of Provider and other components' specifics - thanks for the explanation):

• split isConformant and optional tests (such as implementsShorthandProp ) to separate modules
• use direct module imports

We can reconsider this decision in future as we'll have more mature set of components (in case if necessary), so that it will be easier to make all the necessary analysis for these decisions.

[levithomason]

See comment on chaining and naming.

[kuzhelov]

existing set of tests has been moved to 'SUI' module - lets discuss whether we do need it, as there are some tests that seems to be not relevant for our library