Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: header tests #139

Merged
merged 7 commits into from
Aug 31, 2023
Merged

feat: header tests #139

merged 7 commits into from
Aug 31, 2023

Conversation

tomasciccola
Copy link
Contributor

@tomasciccola tomasciccola commented Aug 28, 2023

This adds some tests for encoding the headers of a MapeoDoc.
Additionally I've added an export for encodingBlockPrefix only to be accessible from the unit test.

This should close #123

@tomasciccola tomasciccola changed the title Feat/header tests feat: header tests Aug 28, 2023
Copy link
Member

@gmaclennan gmaclennan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think these tests still need a bit of work.

For encode/decode of block prefixes that work, we are testing most of the functionality in the tests for good docs. What we aren't testing here that would be good to unit test for is different schema versions, e.g.

const schemaDef = { schemaName: 'project', schemaVersion: 5 }
const buf = encodeBlockPrefix(schemaDef)
t.deepEqual(decodeBlockPrefix(buf), schemaDef)

Although see my test suggestion in the comment for a more thorough version of the above.

It might also be worth testing (to check it ignores data beyond the block prefix):

const schemaDef = { schemaName: 'project', schemaVersion: 5 }
const buf = Buffer.concat(encodeBlockPrefix(schemaDef). randomBytes(50)])
t.deepEqual(decodeBlockPrefix(buf), schemaDef)

Finally worth testing we can encode/decode a schema of version Math.pow(2, 16) -1

And we should check the length of the encoded block prefix is always 8 in the tests above.

For testing when we expect encode or decode to throw:

  1. Calling encodeBlockPrefix with an invalid schemaName
  2. Calling decodeBlockPrefix with a Buffer that is less than the prefix length (8 bytes), e.g.
const schemaDef = { schemaName: 'project', schemaVersion: 5 }
const buf = encodeBlockPrefix(schemaDef).subarray(0, 7)
throws(() => decodeBlockPrefix(buf))
  1. Calling decodeBlockPrefix with random data
  2. Calling decodeBlockPrefix with an empty buffer
  3. Calling decodeBlockPrefix with an unknown data type id e.g.
const schemaDef = { schemaName: 'project', schemaVersion: 5 }
const buf = encodeBlockPrefix(schemaDef)
const unknownDataTypeId = randomBytes(6) // Maybe hard-code, or this will fail with a probability of 7/2^6 
unknownDataTypeId.copy(buf)
throws(() => decodeBlockPrefix(buf))

That's off the top of my head, any other ideas?

src/index.ts Outdated Show resolved Hide resolved
test/index.js Outdated Show resolved Hide resolved
test/index.js Outdated Show resolved Hide resolved
test/index.js Outdated Show resolved Hide resolved
@tomasciccola
Copy link
Contributor Author

Finally worth testing we can encode/decode a schema of version Math.pow(2, 16) -1

whats the reason for this? Is this the biggest schemaVersion we should support?
This would also fail since we're not actually supporting bigger schemaVersion than the currently supported, right??

@gmaclennan
Copy link
Member

whats the reason for this? Is this the biggest schemaVersion we should support?
This would also fail since we're not actually supporting bigger schemaVersion than the currently supported, right??

Because what we think we have done is use 2 bytes (16 bits) to store the schemaVersion - we had that conversation a while ago. We could have chosen 1 byte (schema version up to 256) but we chose 2 bytes to be safe - probably overkill now I think about it! Anyway, given that is what we think we are doing, I thought it's worth testing that is actually the case. This will fail until we fix the bug that does not allow a schemaVersion bigger than currently supported.

Copy link
Member

@gmaclennan gmaclennan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good, good work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add tests for encoding/decoding header on mapeo docs
2 participants