search

kaitai-io / kaitai_struct_javascript_runtime

Kaitai Struct: runtime for JavaScript
Apache License 2.0
37 stars 22 forks source link

Add tests to the KaitaiStream methods and fix 2 bugs #26

Open Mingun opened 2 years ago

Mingun commented 2 years ago

Fixes https://github.com/kaitai-io/kaitai_struct/issues/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 in f379e62 (readS8be / readS8le)

Supersedes and closes #16

Fixes #27

Mingun commented 2 years ago

@generalmimon , could you look at this?

generalmimon commented 2 years ago

It's good that you resolved https://github.com/kaitai-io/kaitai_struct_javascript_runtime/issues/27 in https://github.com/kaitai-io/kaitai_struct_javascript_runtime/pull/26/commits/4a1b441feeee48cda6c4573368d8670a63d4586e, but that commit should ideally contain Fixes #27 or Fixes kaitai-io/kaitai_struct_javascript_runtime#27 (see GitHub Autolinked references and URLs) in the commit message so that the link between the issue and the associated commit is clear (check out git rebase -i <commit-hash>~ and the reword action). I could do it for you, but I don't want to force push to your branch because it could cause you problems.

Also, as follows from the related article https://nodejs.org/en/docs/guides/buffer-constructor-deprecation/#variant-1 (that I also referenced in https://github.com/kaitai-io/kaitai_struct_javascript_runtime/issues/27), using Buffer.from implies dropping support for Node.js ≤ 4.4.x and 5.0.0 — 5.9.x versions (unless we use polyfills which are not worth the trouble). This wouldn't be a problem (if anyone depends on these old versions and cannot/doesn't want to upgrade to newer major ones, they can just update to a newer minor version of Node.js 4 or 5), but ideally we should at least declare this Node.js version requirement in package.json.

Mingun commented 2 years ago

I didn't even know that there was an issue about the deprecated Buffer constructor. Added that information into commit, as well as required node version

Mingun commented 2 years ago

@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)

generalmimon commented 2 years ago

I would be glad if this will be merged

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.

and WebIDE updated (if I remember correctly, devel version is automatically uses the last commit)

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.

Mingun commented 2 years ago

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

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.

If I take a quick look at this PR, there are several things I don't like (mostly about the newly added tests,

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

so shall I cherry-pick the fixes I want and leave the tests rejected?

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.

That's the idea, but the automatic trigger of the Web IDE rebuild is in fact failing for a few months now

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.

Mingun commented 5 months ago

@generalmimon, would you have time to review this PR, or maybe some other of the team can do that?

Mingun commented 5 months ago

https://ci.kaitai.io/ shows that tests are run on Node.js 4. The corresponding lines should be removed when this merged: