chore(tests): cleanup and toolbox drag tests#7350
chore(tests): cleanup and toolbox drag tests#7350rachel-fenichel merged 7 commits intoRaspberryPiFoundation:developfrom rachel-fenichel:test_cleanup
Conversation
BeksOmega
left a comment
BeksOmega
left a comment
There was a problem hiding this comment.
This generally LGTM! There are still lots of things about these tests I'd like to clean up :P but I don't want to block this on those! Feel free to ignore any nitpicky things about formatting/naming.
I am pretty confused about why the tests are skipped though. Is it just while we're building out the tests? I don't think there's anything wrong with them taking a long time, especially since we're only running them nightly.
| if (await isBlockDisabled(browser, i)) { | ||
| // Unicode escape to close flyout. | ||
| await browser.keys(['\uE00C']); | ||
| await browser.pause(PAUSE_TIME); |
There was a problem hiding this comment.
Can you use a continue here instead of wrapping the rest of the method in an else?
There was a problem hiding this comment.
I like else more than continue :)
There was a problem hiding this comment.
I think for long term code health it's better to keep conditional blocks short: https://testing.googleblog.com/2017/06/code-health-reduce-nesting-reduce.html
For smaller things I can see how else is clearer. But in this case (especially since it is already wrapped in a try...catch) I would really prefer to de-dent it.
Yes, for while we're building out the tests. I think our teammates will quickly tire of running the tests locally if they always have to sit through how long these tests take. When we're not actively developing them + have sorted out some of the flakiness, I'll turn these on to run nightly. |
|
@BeksOmega please feel free to leave more comments here telling me what else you would like to see cleaned up, so I can get to them on my return. |
2 participants
The basics
npm run formatandnpm run lintThe details
Resolves
Closes #7327
Closes #7347
Closes #7346
Proposed Changes
In order of commits:
Test Coverage
The toolbox drag tests open every category in the basic toolbox and test blocks toolbox, and drag out every (non-disabled) block.
These tests take a long time (>100 seconds for the test blocks ones) so I have set them to be skipped right now.
The verification step simply checks whether there is a single top block on the workspace after the drag. As written, these tests will tell us about some catastrophic failures but won't guarantee that everything about block initialization works. Having a human watch the tests, with
PAUSE_TIMEset to more than 50 ms, is the best way to actually verify that the blocks that are created look right.In particular, there is a block in the test blocks that has an invalid image URL. Blockly drops an error into the console for that, but the tests don't notice it. Ideally the tests would also monitor the browser console and forward any issues there.
Additional Information
The helpers I added make some strong assumptions about the state of the workspace during the test. I documented the assumptions but did not try to make the helpers more general-purpose. We can generalize them if a specific need comes up.