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

isBinaryFile considers certain protobuf binary files as text #3405

Closed
darrnshn opened this issue Jan 6, 2020 · 4 comments · Fixed by #3422 or karronoli/redpen#10 · May be fixed by Omrisnyk/npm-lockfiles#136, Omrisnyk/npm-lockfiles#137 or Omrisnyk/npm-lockfiles#139
Labels

Comments

@darrnshn
Copy link

darrnshn commented Jan 6, 2020

Expected behaviour

Protobuf binary files considered binary.

Actual behaviour

Protobuf binary files not considered binary.

Environment Details

  • Karma version: v4.4.1
  • Karma config:
module.exports = function(config) {
  config.files.push({
    pattern: 'test/**',
    watched: false,
    served: true,
    nocache: false,
    included: false,
  });
};

Steps to reproduce the behaviour

I don't have a small repro unfortunately, but the root cause is simple. isBinaryFile uses heuristics for detecting binary files. In our case, some of our protobuf files happened to be classified as text even though they are binary, so Karma calls buffer.toString(), which creates a bunch of unicode replacement characters that the protobuf parser can't interpret.

Could we add a way to override the result of isBinaryFile via karma files config? maybe something like:

module.exports = function(config) {
  config.files.push({
    pattern: 'test/**',
    watched: false,
    served: true,
    nocache: false,
    included: false,
    isBinary: true,
  });
};
@Russianze
Copy link

I think for this you need to write a plugin with the desired option, other methods are not available

@johnjbarton
Copy link
Contributor

I think this happens because your "pb" files have a lot of valid ASCII characters so the classifier says "ok looks like extended ASCII".

Unfortunately the buffer.toString() occurs before any preprocessor call so no plugin can fix this easily.

johnjbarton added a commit to johnjbarton/karma that referenced this issue Feb 10, 2020
Add support for isBinary. If set, use it and don't probe file to detect binary status.
Fixes karma-runner#3405
johnjbarton added a commit to johnjbarton/karma that referenced this issue Feb 14, 2020
Add support for isBinary.
If set, use it and don't probe file to detect binary status.

Fixes karma-runner#3405
johnjbarton added a commit that referenced this issue Feb 20, 2020
Add support for isBinary.
If set, use it and don't probe file to detect binary status.

Fixes #3405
johnjbarton added a commit that referenced this issue Feb 20, 2020
Add support for isBinary.
If set, use it and don't probe file to detect binary status.

Fixes #3405
karmarunnerbot pushed a commit that referenced this issue Apr 9, 2020
# [5.0.0](v4.4.1...v5.0.0) (2020-04-09)

### Bug Fixes

* install semantic-release as a regular dev dependency ([#3455](#3455)) ([1eaf35e](1eaf35e))
* **ci:** echo travis env that gates release after_success ([#3446](#3446)) ([b8b2ed8](b8b2ed8))
* **ci:** poll every 10s to avoid rate limit. ([#3388](#3388)) ([91e7e00](91e7e00))
* **middleware/runner:** handle file list rejections ([#3400](#3400)) ([80febfb](80febfb)), closes [#3396](#3396) [#3396](#3396)
* **server:** cleanup import of the removed method ([#3439](#3439)) ([cb1bcbf](cb1bcbf))
* **server:** createPreprocessor was removed ([#3435](#3435)) ([5c334f5](5c334f5))
* **server:** detection new MS Edge Chromium ([#3440](#3440)) ([7166ce2](7166ce2))
* **server:** replace optimist on yargs lib ([#3451](#3451)) ([ec1e69a](ec1e69a)), closes [#2473](#2473)
* **server:** Report original error message ([#3415](#3415)) ([79ee331](79ee331)), closes [#3414](#3414)

### Code Refactoring

* use native Promise instead of Bluebird ([#3436](#3436)) ([33a069f](33a069f)), closes [/github.com//pull/3060#discussion_r284797390](https://github.com//github.com/karma-runner/karma/pull/3060/issues/discussion_r284797390)

### Continuous Integration

* drop node 8, adopt node 12 ([#3430](#3430)) ([a673aa8](a673aa8))

### Features

* **docs:** document `DEFAULT_LISTEN_ADDR` constant ([#3443](#3443)) ([057d527](057d527)), closes [#2479](#2479)
* **karma-server:** added log to the server.js for uncaught exception ([#3399](#3399)) ([adc6a66](adc6a66))
* **preprocessor:** obey Pattern.isBinary when set ([#3422](#3422)) ([708ae13](708ae13)), closes [#3405](#3405)

### BREAKING CHANGES

* Karma plugins which rely on the fact that Karma uses Bluebird promises may break as Bluebird-specific API is no longer available on Promises returned by the Karma core
* **server:** Deprecated createPreprocessor removed, karma-browserify < 7 version doesn't work
* no more testing on node 8.
@karmarunnerbot
Copy link
Member

🎉 This issue has been resolved in version 5.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@larabr
Copy link

larabr commented Aug 15, 2022

I stumbled upon a similar issue: I have a file that can easily be misclassified as non-binary, due to having valid ASCII chars, but it contains some exotic charsets too so it should not be converted to a string under the hood.

I found that the isBinary option is already set up as a Pattern field, but cannot be given under config.files, since it's not passed through as option here. Changing the code to pass through the option seems to work for my use case, and would avoid the need to write a custom middleware that does nothing more than reading the file. Could you add support for setting isBinary as part of the config, as originally suggested?

anthony-redFox pushed a commit to anthony-redFox/karma that referenced this issue May 16, 2023
# [5.0.0](karma-runner/karma@v4.4.1...v5.0.0) (2020-04-09)

### Bug Fixes

* install semantic-release as a regular dev dependency ([karma-runner#3455](karma-runner#3455)) ([1eaf35e](karma-runner@1eaf35e))
* **ci:** echo travis env that gates release after_success ([karma-runner#3446](karma-runner#3446)) ([b8b2ed8](karma-runner@b8b2ed8))
* **ci:** poll every 10s to avoid rate limit. ([karma-runner#3388](karma-runner#3388)) ([91e7e00](karma-runner@91e7e00))
* **middleware/runner:** handle file list rejections ([karma-runner#3400](karma-runner#3400)) ([80febfb](karma-runner@80febfb)), closes [karma-runner#3396](karma-runner#3396) [karma-runner#3396](karma-runner#3396)
* **server:** cleanup import of the removed method ([karma-runner#3439](karma-runner#3439)) ([cb1bcbf](karma-runner@cb1bcbf))
* **server:** createPreprocessor was removed ([karma-runner#3435](karma-runner#3435)) ([5c334f5](karma-runner@5c334f5))
* **server:** detection new MS Edge Chromium ([karma-runner#3440](karma-runner#3440)) ([7166ce2](karma-runner@7166ce2))
* **server:** replace optimist on yargs lib ([karma-runner#3451](karma-runner#3451)) ([ec1e69a](karma-runner@ec1e69a)), closes [karma-runner#2473](karma-runner#2473)
* **server:** Report original error message ([karma-runner#3415](karma-runner#3415)) ([79ee331](karma-runner@79ee331)), closes [karma-runner#3414](karma-runner#3414)

### Code Refactoring

* use native Promise instead of Bluebird ([karma-runner#3436](karma-runner#3436)) ([33a069f](karma-runner@33a069f)), closes [/github.com/karma-runner/pull/3060#discussion_r284797390](https://github.com//github.com/karma-runner/karma/pull/3060/issues/discussion_r284797390)

### Continuous Integration

* drop node 8, adopt node 12 ([karma-runner#3430](karma-runner#3430)) ([a673aa8](karma-runner@a673aa8))

### Features

* **docs:** document `DEFAULT_LISTEN_ADDR` constant ([karma-runner#3443](karma-runner#3443)) ([057d527](karma-runner@057d527)), closes [karma-runner#2479](karma-runner#2479)
* **karma-server:** added log to the server.js for uncaught exception ([karma-runner#3399](karma-runner#3399)) ([adc6a66](karma-runner@adc6a66))
* **preprocessor:** obey Pattern.isBinary when set ([karma-runner#3422](karma-runner#3422)) ([708ae13](karma-runner@708ae13)), closes [karma-runner#3405](karma-runner#3405)

### BREAKING CHANGES

* Karma plugins which rely on the fact that Karma uses Bluebird promises may break as Bluebird-specific API is no longer available on Promises returned by the Karma core
* **server:** Deprecated createPreprocessor removed, karma-browserify < 7 version doesn't work
* no more testing on node 8.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment