Adds Error message for when there are no screenshots #6773
Conversation
…r might need to wait for a promise
test/unit/visual/visualTest.js
Outdated
| @@ -151,6 +151,12 @@ window.visualTest = function( | |||
| actual.push(myp5.get()); | |||
| }); | |||
|
|
|||
|
|
|||
| if (actual.length === 0) { | |||
| throw new Error('No screenshots were generated. Check if your test generates screenshots correctly.If the test includes asynchronous operations, ensure they complete before the test ends.'); | |||
There was a problem hiding this comment.
Looks like a space is missing here:
| throw new Error('No screenshots were generated. Check if your test generates screenshots correctly.If the test includes asynchronous operations, ensure they complete before the test ends.'); | |
| throw new Error('No screenshots were generated. Check if your test generates screenshots correctly. If the test includes asynchronous operations, ensure they complete before the test ends.'); |
contributor_docs/unit_testing.md
Outdated
|
|
||
|
|
||
| The visual test environment is set up to execute your commands sequentially rather than running a preload or draw function that you provide. | ||
| In continuous integration (CI) environments, it's crucial to keep tests running as quickly as possible. Running a full `preload/draw` cycle as in a regular p5.js sketch can significantly slow down the testing process. |
There was a problem hiding this comment.
It's not quite that the preload/draw cycle takes long, but we want to encourage the code to be as minimal as possible, which generally means not drawing multiple frames when just one will do, using a small canvas size, and only loading assets if required by the functionality being tested. We maybe don't have to capture all of that in this one sentence, but maybe we can rephrase slightly?
There was a problem hiding this comment.
In a continuous integration (CI) environment, optimizing test speed is essential. It is advantageous to keep the code concise, avoid unnecessary frames, minimize canvas size, and load assets only when essential for the specific functionality under test.
@davepagurek Let me know if the above paragraph would make more sense or something else.
contributor_docs/unit_testing.md
Outdated
|
|
||
| The visual test environment is set up to execute your commands sequentially rather than running a preload or draw function that you provide. | ||
| In continuous integration (CI) environments, it's crucial to keep tests running as quickly as possible. Running a full `preload/draw` cycle as in a regular p5.js sketch can significantly slow down the testing process. | ||
| When testing features like 3D model rendering, you might encounter scenarios where you need to load a model before performing assertions. Here's an example of how you can handle Sequential Command Execution and Asynchronous Operations in your visual tests: |
There was a problem hiding this comment.
Rather than saying "Sequential Command Execution and Asynchronous Operations", it might be simpler to mention something like, return a promise that resolves when you have finished everything you want to test
There was a problem hiding this comment.
To address scenarios involving operations like 3D model rendering, consider returning a promise that resolves upon completing all the necessary tests, ensuring efficiency in your visual testing approach.
@davepagurek Does this make more sense? Do we need more modifications? Maybe remove mentioning 3D models?
There was a problem hiding this comment.
Maybe instead of "3D model rendering" you could say "asynchronous 3D model loading"? Since a promise is only necessary for async things, which is mostly loading. Other than that, the rest looks good!
There was a problem hiding this comment.
Yes, Thanks that would make it much more clear. @davepagurek
|
@davepagurek Let me know if we need to make further changes. |
Resolves #6772
Changes:
test/unit/visual/visualTest.jsifactual.length===0added error handling to throw an Error indicating lack of screenshots or if the developer needs to wait for asynchronous tasks to complete before the test ends.Screenshots of the change:
PR Checklist
npm run lintpasses