Add unit tests for AnimatedSprite#4744
Conversation
There was a problem hiding this comment.
You're on a roll @schipiga. Great addition. It's really good for us to get more coverages on classes like this. Event just basic coverage.
The only thing I would suggest is not checking private properties (ones starting with _), it's more useful to only check the pubic APIs. The result makes for a less brittle test. There are some cases where evaluating a private property is very useful and necessary. In the case of AnimatedSprite and this PR, I'd remove some of the expect lines.
Also, might be useful to ada few more tests to stub out functionality of onComplete, onLoop, etc methods.
| expect(this.sprite._currentTime).to.be.equal(0); | ||
| expect(this.sprite.playing).to.be.false; | ||
|
|
||
| this.sprite.destroy(); |
There was a problem hiding this comment.
Make sure you set this.sprite = null after the destroy
There was a problem hiding this comment.
Btw we dont need destroy() there, because it really doesnt appear in caches and has no generated textures.
There was a problem hiding this comment.
anyway, just add "this.sprite=null" and it'll be fine.
There was a problem hiding this comment.
@ivanpopelyshev maybe stay destroy() anycase for reliability? if in future it will be meaningful we will not need to update tests.
There was a problem hiding this comment.
Leave destroy, it doesn’t hurt
|
@bigtimebuddy thank you for review, I will fix it a bit later |
|
That's correct @bigtimebuddy, unit tests by design SHOULD cover only public methods. |
@cursedcoder, if you talk about PixiJS design, so maybe. But commonly it depends on context. |
025b1b6 to
5d91791
Compare
5d91791 to
e209623
Compare
|
@bigtimebuddy could you please review it again? |
There was a problem hiding this comment.
This looks like a good start to me. Again, if we were to add more tests in the future for this class, they might include:
- Testing currentFrame numbers with gotoAndStop, gotoAndPlay
- Testing manual update vs autoUpdate
- Testing onComplete, onFrameChange, onLoop
- Testing animationSpeed
Doesn't need to be in this PR. We can add as a separate PR
|
okey :) |
|
How enter in project? Im Brazil :) |
|
Thanks for all your hard work on these unit-tests @schipiga, sorry they took so long to get in. |
|
This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
No description provided.