Inserter Menu: Add unit tests #2018
Conversation
youknowriad
added
the
[Type] Automated Testing
editor/inserter/test/menu.js
Outdated
| ); | ||
| const visibleBlocks = wrapper.find( '.editor-inserter__block' ); | ||
| expect( visibleBlocks.length ).toBe( 1 ); | ||
| expect( visibleBlocks.text() ).toBe( '<Dashicon />Advanced Text' ); |
There was a problem hiding this comment.
This fails after rebase because the Dashicon was replaced with BlockIcon in #1870. Also the way this string is formed seems potentially fragile (what if future serializers omit the space before the closing slash?). Maybe: find, contains, childAt, or not testing content at all.
expect( visibleBlocks.childAt( 0 ).name() ).toBe( 'BlockIcon' );
expect( visibleBlocks.childAt( 1 ).text() ).toBe( 'Advanced Text' );
editor/inserter/test/menu.js
Outdated
| /> | ||
| ); | ||
| const visibleBlocks = wrapper.find( '.editor-inserter__block' ); | ||
| expect( visibleBlocks.length ).toBe( 1 ); |
There was a problem hiding this comment.
I'm finding when I run this test in isolation (adding .only), this line fails because it's expecting 36 as length... sounds like the default set of registered blocks? Is this bleeding over from another test? Should we unregister all blocks in a beforeAll block here?
There was a problem hiding this comment.
I wasn't able to use the .only for some reason but I made the changes that should fix it.
youknowriad
force-pushed
the
test/inserter-menu
branch
from
753e5f7 to
bbf58c7
Compare
editor/inserter/test/menu.js
Outdated
| /> | ||
| ); | ||
|
|
||
| expect( enzymeToJson( wrapper ) ).toMatchSnapshot(); |
There was a problem hiding this comment.
I don't like these assertions personally, they break on each small change to the component. I'd rather add more targeted tests
|
Thanks for working on this. I added a test for searching and took the opportunity to try out Jest snapshots. It will make it easier to write tests, but may cause them to be updated more frequently. They're easy to update with Further reading on the tradeoffs here:
For the |
|
I don't think snapshots are good for testing Markup or components output, Components are updated frequently. I'd rather use the snapshots for things like "serializing","parsing" where we're not supposed to change the output so often. Another downside is the maintenance of the test, with the snapshot we don't know exactly what part of the markup is being tested exactly. When the test breaks we don't know if it's really a bug or we just have to update the generated markup. The snapshot is not tied to its assertion. |
|
I think it's a question of which way is ultimately more effort:
Still, maybe snapshots would be a better idea later on in the life of the project when it is more stable, and we should revisit then. |
nylen
changed the title
This reverts commit 23b2be06ea85bfe711911d5b7572eab0fb4ef623. Conflicts: editor/inserter/test/menu.js
This reverts commit 37d546919981419b108dee4e74586df378f5de89.
nylen
force-pushed
the
test/inserter-menu
branch
from
940de1a to
3fe2630
Compare
Codecov Report
@@ Coverage Diff @@
## master #2018 +/- ##
==========================================
+ Coverage 18.82% 20.27% +1.44%
==========================================
Files 129 130 +1
Lines 4197 4198 +1
Branches 716 715 -1
==========================================
+ Hits 790 851 +61
+ Misses 2868 2819 -49
+ Partials 539 528 -11
Continue to review full report at Codecov.
|
|
☝️ This is pretty cool IMO, and more useful and less noisy than the previous service (it will only leave one comment, then edit it afterwards). Thoughts on whether we want to leave it enabled? |
|
@nylen I agree, this is great 👍, you see exactly what you improved or not |
|
Okay. I do think the tests are nicer this way too, it's just a bit annoying to write. Let's merge this 👍 |
Follow up to #1661
Several things could be tested for the inserter menu, this PR bootstraps a basic unit test for this component:
useOnce: trueis disabled.