Skip to content

Conversation

@salesh
Copy link
Contributor

@salesh salesh commented Feb 25, 2023

#582
Improving test coverage part 1

Checklist

if (largeArrayMechanism !== 'default') {
if (largeArrayMechanism === 'json-stringify') {
functionCode += `if (arrayLength && arrayLength >= ${largeArraySize}) return JSON.stringify(obj)\n`
} else {
Copy link
Contributor Author

@salesh salesh Feb 25, 2023

Choose a reason for hiding this comment

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

because validLargeArrayMechanisms -> this throw will never happen (we get an early throw if largeArrayMechanism is not default or json-stringify) so no need for it -> already covered by tests

Copy link
Contributor

Choose a reason for hiding this comment

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

thats true

return buildArrayTypeCondition(subType, accessor)
})
condition = `(${conditions.join(' || ')})`
} else {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this case is not possible because validation of types schema is invalid: data/properties/ids/type must be equal to one of the allowed values -> already covered by tests

Copy link
Contributor

Choose a reason for hiding this comment

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

which line is already handling this already?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

function isValidSchema (schema, name) {
whenever i try to set something for the type that is not in that switch

Copy link
Contributor Author

Choose a reason for hiding this comment

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

concrete line

isValidSchema(options.schema[key], key)

@salesh
Copy link
Contributor Author

salesh commented Feb 25, 2023

serializer.js: 98.85 -> 100%
index.js: 97.62% -> 98.13%

Copy link
Member

@ivan-tymoshenko ivan-tymoshenko left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@mcollina mcollina merged commit 43f5ee9 into fastify:master Feb 25, 2023
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.

4 participants