Add basic Spinner component tests. #936
Conversation
BE-Webdesign
added
the
[Type] Automated Testing
| it( 'should match the following element', () => { | ||
| const element = <span className="spinner is-active" />; | ||
| const spinner = shallow( <Spinner /> ); | ||
| expect( spinner.matchesElement( element ) ).to.be.true(); |
There was a problem hiding this comment.
Where do we put the limit between testing the markup or not?
I think you're doing an awesome job testing all these components, but do we really need to add this test specifically? Does it avoid any regression, what's the value here since we're basically copy/pasting the content of the component here?
There was a problem hiding this comment.
I have found tests like this useful when I change markup in a component and forget to update the styles. When things fail it reminds me to update the style. I can understand them not being useful persee, but I typically aim for 100% coverage. On projects that move relatively fast like this, I think more tests over fewer tests is better. It should probably be decided what we are going to test, how, and why.
There was a problem hiding this comment.
I opened up #939 for discussion! Add your thoughts there.
There was a problem hiding this comment.
These kind of tests are also useful for when you need to do upgrades, so you can know a lot of what is broken from the unit tests themselves.
Adds basic Spinner component tests. Related to progress on #641. Testing Instructions Run npm i && npm run test-unit ensure tests pass. Change component logic to ensure tests fail as they should.
BE-Webdesign
force-pushed
the
add/test/components/spinner
branch
from
cdf0b29 to
effef54
Compare
|
Any oppositions to merge? |
|
Do you find #939 to have settled previous concerns? It's a strange case, since the component really has no variation and the logic is minimal. Seems most we care to do is expect it returns something, which I suppose the tests here are doing. To the granularity that we care to test markup, I don't really know where we draw the line. I think most value would come from if we expect different props (inputs) to affect output, which we don't have here. I'm mostly indifferent. Personally I'd be fine if this component had no tests at all in its current form, but I don't think there's much harm in including them, aside from precedent in how to handle such scenarios in the future if we worry that it introduces more overhead than it provides benefit. |
|
Closing as it doesn't provide much benefit at this time. Nor will it most likely in the future. |
Adds basic Spinner component tests. Related to progress on #641.
Testing Instructions
Run npm i && npm run test-unit ensure tests pass. Change component logic
to ensure tests fail as they should.