Add testing for primitives, objects and tools #20
Conversation
|
Hey @tmadlener, the preview didn't work. I don't know why. Just to make sure, did you change the workflow permission like this? |
|
The settings should be OK as far as I can see |
|
Ok. Then I will finish writing more tests and later open a new PR to fix the previews. |
brauliorivas
changed the title
There was a problem hiding this comment.
Very nice. I think this is OK as it is in principle. One thing that I would consider to do is the following.
Currently all the individual test cases (or test case sections? 🤔 ) create their own objects to test manually. Quite often they create the same object. Is it possible to lift this code outside of the actual cases and create a fixture (or use some other setup method) that will then be freshly created automatically for each test?
I like the thoroughness of the tests, and I think it's OK for now, but in general some of them are very detailed and start to test implementation details (IMHO). Especially some of the primitives.test.js ones. Tests that check for implementation details, rather than the interfaces / "visible behavior" are usually hard to maintain because whenever your implementation changes, also your tests have to change. However, most of the times as long as the visible behavior is unchanged, it doesn't really matter how that is achieved.
I think for now we can keep this and if we realize that they start to become a nuisance we can think about how to change them once we are at that point.
Finally, as far as I can see these are not yet automatically run in a workflow, right? Can you add the necessary configuration for that?
test/objects.test.js
Outdated
| const result = infoBox.isHere(x, y); | ||
|
|
||
| expect(result).toBe(false); |
There was a problem hiding this comment.
| const result = infoBox.isHere(x, y); | |
| expect(result).toBe(false); | |
| expect(infoBox.isHere(x, y)).toBe(false); |
Would make the cases a few lines shorter. Similar for other cases below.
package.json
Outdated
There was a problem hiding this comment.
Can you make this a proper package.json with name and license,....
.github/workflows/test.yml
Outdated
| @@ -0,0 +1,22 @@ | |||
| name: Run test for dmX on push to main and PR's | |||
There was a problem hiding this comment.
| name: Run test for dmX on push to main and PR's | |
| name: Build and test |
Just make this a bit shorter to make the check section of the PRs a bit easier to read. The other information is not really necessary, since we know in which repository we are and in the end we don't care too much what triggered the workflow.
tmadlener
force-pushed
the
test-primitives
branch
from
c110c4d to
5c85df6
Compare
|
BEGINRELEASENOTES
ENDRELEASENOTES
I checked a library called jest-canvas-mock that allows to test the canvas API using jest. However after trying to use it a bit, I don't think that can be useful. There is almost no documentation, and some examples try to test for error rather than to test visual aspect. I don't think this meets testing goal, to be simple and cover an important use case. So that's why I think we could use plain jest with a less strict approach but easier to test.