-
-
Notifications
You must be signed in to change notification settings - Fork 375
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
Prevent size related exceptions #307
Conversation
I am not really happy with way the await tokenizer.peekBuffer(buffer, 0, bytesRead, tokenizer.position, true); //maybeless = true I have created an issue: strtok3#124 |
await FileType.fromBuffer(buf); | ||
}, `file size = ${size} bytes`); | ||
const buffer = Buffer.alloc(size); | ||
await t.notThrowsAsync(FileType.fromBuffer(buffer), `File size: ${size} bytes`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have no idea why this is failing:
✖ odd file sizes
t.notThrowsAsync()
must be called with a function or promise
FileType.fromBuffer(buffer)
is a Promise.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am happy I am not the only one anymore who thinks the behavior of the async tests methods are weird. Passing a promise should be sufficient.
test.js is full of redundant await
statements.
I think in general async tests methods & await statements could be avoided by treating the test method async if the return type of the test method is a promise.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I know why it happens. When the file type cannot be detected, .fromBuffer()
incorrectly returns undefined
instead of Promise<undefined>
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aij
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is missing async
declaration:
Line 23 in e90035a
function fromBuffer(input) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will create separate issue / PR for that. I am curious if the test cases are then working with a single await as well.
... issue created: #309
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And indeed, with the async fix, the async-test- methods do behave as expected 😁
2a431bd
to
7ce5d76
Compare
Force pushed a rebase on PR #310. |
7ce5d76
to
ed01794
Compare
Prevent exceptions: