Validate b3dm new #16
Conversation
| it('validated is a b3dm tile', function(done) { | ||
| expect(validateB3dm('./specs/data/Tileset/parent.b3dm')).toBe(true); | ||
| expect(validateB3dm(createB3dmTile())).toBe(true); |
There was a problem hiding this comment.
for these lines, I'm geting the "Asyc callback was not invoked within timeout specified by jasmne.DEFAULT_TIMEOUT_INTERVEL" error.
I get that I never explicitly called a callback I guess, but realistically, all I needed to do is verify that it is true or false so I tried to follow:
http://jasmine.github.io/2.2/introduction.html
I'm not sure what to put in then() or what to promisify to get a promise (I'm still trying to fully understand promises and promisify).
There was a problem hiding this comment.
Just remove the done parameter for each spec, this is only needed when the test is asynchronous.
|
I removed the done parameter and just ran the tests. Everything passed. Please let me know if you notice anything that still needs to be changed. |
| * {@link https://github.com/AnalyticalGraphicsInc/3d-tiles/tree/master/TileFormats/Batched3DModel}, False if not. | ||
| */ | ||
| function validateB3dm(content) { | ||
| if(!Cesium.defined(content)) { |
There was a problem hiding this comment.
Add var defined = Cesium.defined at the top instead.
Also add one space between the if and (, just to keep consistency, here and below.
| } | ||
|
|
||
| return true; | ||
| } |
| var validateB3dm = require('../../lib/validateB3dm'); | ||
|
|
||
| describe('validateB3dm', function() { | ||
|
|
| }); | ||
|
|
||
| function createB3dmTile() { | ||
|
|
There was a problem hiding this comment.
Remove whitespace here too, and for the other functions.
|
|
||
| var header = new Buffer(24); | ||
|
|
||
| header.write('b3bm', 0); // magic |
There was a problem hiding this comment.
Can you change this to xxxx or something a little more obvious, for a while I was trying to figure out why this test was passing, before I realized.
| header.writeUInt32LE(0, 20); // batchLength | ||
|
|
||
| return header; | ||
| } |
There was a problem hiding this comment.
This seems to be a duplicate of the function above.
| expect(validateB3dm(createInvalidVersion())).toBe(false); | ||
| }); | ||
|
|
||
| it('validated not b3dm tile, wrong byteLength', function() { |
There was a problem hiding this comment.
These test descriptions are a bit awkward. Instead try:
returns true if the b3dm tile is valid, returns false if the b3dm has invalid magic, etc.
|
That's my last comment for this. I'll start opening some new issues for the other tile types. |
|
Looks good @JudyWeng! Also in the future make sure to leave a comment after updating so I know the PR is ready. |
| var byteLength = content.readUInt32LE(8); | ||
|
|
||
| if (magic !== 'b3dm') { | ||
| return false; |
There was a problem hiding this comment.
Are we sure we just want to return a boolean? Wouldn't we want the user to have a precise error message for which field is the issue?
There was a problem hiding this comment.
Hm yes and we are doing something similar for validating the tileset #15. @JudyWeng can you open a new PR that fixes this here?
Something simple like:
if (magic !== 'b3dm') {
return {
result : false,
message : 'Invalid magic'
};
}Also edit the jsDoc so it returns an Object instead of a Boolean. You can look at the code in #15 as an example.
continuing from #14