-
Notifications
You must be signed in to change notification settings - Fork 22
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
Add tests to the KaitaiStream methods and fix 2 bugs #26
base: master
Are you sure you want to change the base?
Conversation
@generalmimon , could you look at this? |
It's good that you resolved #27 in 4a1b441, but that commit should ideally contain Also, as follows from the related article https://nodejs.org/en/docs/guides/buffer-constructor-deprecation/#variant-1 (that I also referenced in #27), using |
I didn't even know that there was an issue about the deprecated |
@generalmimon, I would be glad if this will be merged and WebIDE updated (if I remember correctly, devel version is automatically uses the last commit) |
I understand, but I can't promise anything. I'm currently already well behind schedule with things I need to do. I may have a weak moment some evening, but don't hold your breath. I'm not that eager to reviewing your pull requests because 1. doing a dependable review takes me a lot of time (I don't think people realize that, it must probably be a seamless instantaneous process, or maybe there's really no need for reviewing anything and let's just jump right into merging something with obvious problems), 2. your contributions tend to be rather low-priority (don't get me wrong, it'd be nice to have them even though they're often unnoticeable by the end user, and it would also be nice if I had unlimited time to deal with all PRs in all kaitai-io repos and it wouldn't be at the expense of KS development itself), 3. chances are I'll suggest to improve something and you'll start arguing with me about how baseless it is and how I'm a horrible human being, and I certainly don't have time for such arguments. If I take a quick look at this PR, there are several things I don't like (mostly about the newly added tests, but you again packed it in a PR together with the fixes I would accept after some work, so shall I cherry-pick the fixes I want and leave the tests rejected? I don't know...). I can already imagine you wanting to argue with me when I mention them, and I don't want to be part of that argument.
That's the idea, but the automatic trigger of the Web IDE rebuild is in fact failing for a few months now (https://app.travis-ci.com/github/kaitai-io/kaitai_struct/builds/253883384#L1152-L1193), and I haven't yet managed to find out how to fix it. We've been using the Travis CI API V3 all this time (https://github.com/kaitai-io/kaitai_struct/commits/master/trigger-kaitai_struct_webide), but at some point the server apparently started rejecting it with 403 Forbidden for some reason. |
Then maybe think about increasing pool of people who have the rights to push merge button? Create an issue with an invitation to become a maintainer would be a good start and I am sure you will receive enough responses.
I'm wonder, what's wrong with tests, whereas these are the only tests here and they are as straightforward as possible. Do you mean that it would be better to not test anything? But the bugs fixed appeared just because functions was not tested
Yes, please, do that if you like that. In the end, a fixed bug is better than everything else in case when the project practically does not accept PRs.
It's depressing. Another consequence of the ill-conceived policy of accepting contributors to the project. One or two people simply cannot keep track of everything if only because it is impossible to be an expert in many languages at once. I hope you seriously consider the possibility of the project becoming more open to community. |
Failures (6): KaitaiStream × readS8be (3 ms) × readS8le (1 ms) readBytesTerm when terminator is present in an input × include = true, consume = false, eosError = false (1 ms) × include = true, consume = false, eosError = true × include = true, consume = true, eosError = false (1 ms) × include = true, consume = true, eosError = true (1 ms)
This fixes kaitai-io/kaitai_struct#783 for JavaScript (4 failures) Failures (2): KaitaiStream × readS8be (27 ms) × readS8le (1 ms)
This commit fixes 2 failures
Fixes kaitai-io#27 Declare minimum supported node version as 6 according to https://nodejs.org/en/docs/guides/buffer-constructor-deprecation/#variant-1
@generalmimon, would you have time to review this PR, or maybe some other of the team can do that? |
https://ci.kaitai.io/ shows that tests are run on Node.js 4. The corresponding lines should be removed when this merged: |
Fixes kaitai-io/kaitai_struct#783. This is the first bug, it is present only in JavaScript runtime.
The second fixed bug is an incorrect reading of negative
s8
numbers. See the failed tests inf379e62 (
readS8be
/readS8le
)Supersedes and closes #16
Fixes #27