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

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

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

Mingun
Copy link

@Mingun Mingun commented Dec 12, 2021

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 in
f379e62 (readS8be / readS8le)

Supersedes and closes #16

Fixes #27

@Mingun
Copy link
Author

Mingun commented Apr 23, 2022

@generalmimon , could you look at this?

@generalmimon
Copy link
Member

generalmimon commented Apr 23, 2022

It's good that you resolved #27 in 4a1b441, 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 #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
Copy link
Author

Mingun commented Apr 23, 2022

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
Copy link
Author

Mingun commented Aug 11, 2022

@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
Copy link
Member

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
Copy link
Author

Mingun commented Aug 12, 2022

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.

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
@Mingun
Copy link
Author

Mingun commented Apr 5, 2024

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

@Mingun
Copy link
Author

Mingun commented Apr 5, 2024

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