test: add tests for the insertion marker manager #6596
Conversation
There was a problem hiding this comment.
Generally looks really good! Unit tests make me so happy haha.
One high-level thing: How would you feel about renaming some stuff? I really like this convention, because I think it makes it very clear what each suite/test is doing:
- suites: This tests '___' E.g.
suite('Creating markers', function() { }) - tests: This tests that '___' E.g.
test('a single stack block results in a single marker block', function() { });
[Edit: one other high-level thing, it looks like the indentation is using 4 spaces for this file]
| }; | ||
| const manager = createBlocksAndManager(this.workspace, state); | ||
| const markers = manager.getInsertionMarkers(); | ||
| chai.assert.equal(markers.length, 1); |
There was a problem hiding this comment.
Not a comment about the contents of the test (that looks great!), but why is this the intended behavior?
There was a problem hiding this comment.
If you have multiple row blocks in a row, we don't offer to connect to the end of the row.
I don't know why, though. Some things that come to mind:
- Value inputs/outputs often have type checks, so often it's not allowed anyway
- Distance from mouse pointer gets large fast (when compared to stacks)
| chai.assert.equal(markers.length, 1); | ||
| }); | ||
|
|
||
| test('Two stack blocks', function() { |
There was a problem hiding this comment.
How does this handle stacks where the top block in the stack has an ouput instead of a previous connection?
There was a problem hiding this comment.
Two markers! I'll have to add a different test block for that.
And if the output + next block is the only block in the stack, it's one marker (I think).
| this.manager.update(new Blockly.utils.Coordinate(200, 190), null); | ||
| chai.assert.isTrue(this.manager.wouldConnectBlock()); | ||
| const markers = this.manager.getInsertionMarkers(); | ||
| chai.assert.equal(markers.length, 1); |
There was a problem hiding this comment.
What are these assertions checking? These look like dupes of the 'Create markers' assertions, can they be removed?
There was a problem hiding this comment.
Checking that it's safe to call markers[0].nextConnection.isConnected. But I can remove the assertions on markers.length and assume that if I'm wrong the test will still fail on the next access. What do you think?
There was a problem hiding this comment.
Re: naming convention: I'm not a huge fan, because it leads to really verbose names of the tests, especially when they're doing similar things. I'm mostly resigned to using the name of the test as a hint, but needing to read it to really understand what it's testing.
| }; | ||
| const manager = createBlocksAndManager(this.workspace, state); | ||
| const markers = manager.getInsertionMarkers(); | ||
| chai.assert.equal(markers.length, 1); |
There was a problem hiding this comment.
If you have multiple row blocks in a row, we don't offer to connect to the end of the row.
I don't know why, though. Some things that come to mind:
- Value inputs/outputs often have type checks, so often it's not allowed anyway
- Distance from mouse pointer gets large fast (when compared to stacks)
| this.manager.update(new Blockly.utils.Coordinate(200, 190), null); | ||
| chai.assert.isTrue(this.manager.wouldConnectBlock()); | ||
| const markers = this.manager.getInsertionMarkers(); | ||
| chai.assert.equal(markers.length, 1); |
There was a problem hiding this comment.
Checking that it's safe to call markers[0].nextConnection.isConnected. But I can remove the assertions on markers.length and assume that if I'm wrong the test will still fail on the next access. What do you think?
| chai.assert.equal(markers.length, 1); | ||
| }); | ||
|
|
||
| test('Two stack blocks', function() { |
There was a problem hiding this comment.
Two markers! I'll have to add a different test block for that.
And if the output + next block is the only block in the stack, it's one marker (I think).
There was a problem hiding this comment.
Re: naming convention: I'm not a huge fan, because it leads to really verbose names of the tests, especially when they're doing similar things. I'm mostly resigned to using the name of the test as a hint, but needing to read it to really understand what it's testing.
I'm gonna push back against this one more time, (this time with evidence!) but also approving so you can go ahead and merge if you really disagree =)
rachel-fenichel
force-pushed
the
imm_tests
branch
from
0c39a77
to
7942703
Compare
|
Okay I updated test names and also added tests for the row to stack block. |
The basics
npm run formatandnpm run lintThe details
Resolves
Just improving test coverage.
Proposed Changes
Add tests.
Test Coverage
There are four test suites.
The first one checks that the constructor works when given various blocks and block stacks, and that the appropriate number of markers is created (i.e. one marker for the first block, and sometimes another for the last block in the stack).
The second suite checks logic controlling whether a block is deleted when the drag ends. I stubbed the capability manager and drag target to force it down various paths.
wouldDeleteBlockis one of the few public methods available to get information out of the insertion marker manager.The third and fourth suites check that the insertion marker manager can find an appropriate connection in the middle of a drag. I faked dragging the block above, below, left, and right from the stationary block, and then checked the connections on the marker to make sure it was connected in the correct place.
Documentation
No documentation needed.
Additional Information
These tests will hopefully catch any bugs in an upcoming cleanup of the insertion marker manager.
Live footage of me working on this PR:
