Fix shader comparison #297
Conversation
|
Doesn't this change imply we're missing some pretty critical unit tests? We've had several problems in this area. |
|
The spec for |
Sounds like we have bad specs as well then. We should be mocking/spying on as little as possible in our specs precisely because it leads to problems like this. |
|
Thanks for pinpointing the issue @likangning93. Fixed the test. It passed before because shaders referenced the same Buffer objects which would pass both the |
|
@likangning93 if this looks good can you merge? Then I'll push a new release out. |
| @@ -139,8 +139,6 @@ describe('MergeDuplicateProperties', function() { | |||
|
|
|||
| var mergeShaders = MergeDuplicateProperties.mergeShaders; | |||
| describe('mergeShaders', function() { | |||
| var testShaderBufferOne = Buffer.from('test shader one', 'utf8'); | |||
| var testShaderBufferTwo = Buffer.from('test shader two', 'utf8'); | |||
| it('merges duplicate shaders', function() { | |||
There was a problem hiding this comment.
This test is basically useless, why doesn't it should load and use real gltf files, not mock what it expects the source to look like.
Given you want to get out a new release, I'm okay with @likangning93 merging this, but I'll write up an issue to fix this and other tests that do this.
There was a problem hiding this comment.
I agree here. In general we have strayed away from creating too many test gltf files on disk, but maybe a middle ground is good.
|
Submitted #300 |
Solves the crash in #296, but not the conversion problem
@mramato