Continuous Integration

[thisconnect]

Tests have top priority to ensure backwards compatibility. Test should run in node and the browser(s) and shuold be setup with CI (travis, appveyor?)

mocha, jasmine, ava, tape, jest (others?) which one should be used to jimp?

Should karma (or something else) be used to run the test in real browsers?

[aurium]

I'm comfy with mocha. Karma may be nice, however mocha is pretty good on browsers too. If we discover that we need karma, it support mocha tests, so, i believe it's ok to start writing mocha testes. And that is what i'll do now. What you think about?

[aurium]

Also, we may think in something like the XPM image format to feed our tests with text friendly image information, aiming to use less file samples, and simplify tests creation and understanding.

[thisconnect]

I personally use tape a lot if it has to run in the browser and on Node.js. Mocha is well established, widely used and fine, I guess. I just never liked that there are globals introduced by mocha, that's all and no biggie. I look forward seeing a simple test with mocha + xpm image format, never heard about that before. Should we close this issue or keep it open?

[aurium]

Keep it open while the new test infra is not done. Keep throwing ideas!

I use shouldjs to better assertion. What is your opinion? sinonjs is also good mocking/stubbing lib to work with shouldjs. However i think the simpler spies could do the stubbing job on this project.

[thisconnect]

At work they use sinonjs though I prefer simplicity over sinonjs api.

Some projects use benchmark / https://benchmarkjs.com/ to benchmark against a different version of the library. What do you think about benchmark?

[aurium]

I'm new to benchmarkjs, still I like it. @Iwasawafag did show me this lib.

[aurium]

Folks, please see my changes and additions at https://github.com/oliver-moran/jimp/tree/tests Until now i try to don't change the Jimp behavior or structure, but only the necessary to validate with lint and enable testing.

I did implement only two simple tests until now to validate the test framework. Please, help me to ensure is it all ok to merge the test branch on master as sooner as possible.

[iwsfg]

I'm not a big fan of presenting code style through ESLint. Something is missing here. An extensive contribution guide perhaps. Here are my biggest concerns:

• Why linting rules aren't based on existing code style? I noticed you changed indentation of switch case blocks in code, rather than changing configuration of the linter. indent rule is configurable
• I don't think linebreak-style rule should be enforced. For many users on Windows autocrlf option in git config is set to true. In fact I even think it's a default/recommended option. Maybe change it to a warning? I feel like that's going to be a pain for many Windows users when testing locally all though I'd like this rule to be enforced on CI's side.

Would also love to know a reason for accepting arguments like this

is it all ok to merge the test branch on master as sooner as possible.

Linting of /tools/browser-build.js fails on no-console rule, You're probably aware of that, but that needs to be fixed before we could merge. Tetsts shouldn't fail from the start

I've not yet gone through all changes in index.js, but there are also few questions I would like to ask.

• It's not a ES6 code, why some var-s was replaced with let-s? Not all though.
• Can JGD support be added with a wrapper? image/jgd is not a real MIME-type. I don't see much value in this for a regular user and it makes understanding of the main library's code little harder. Is it necessary to handle that as one of the image "formats"?
• indentation was not corrected in some places 1, 2, 3

I'll take another look at your branch this evening

[aurium]

Thank you @Iwasawafag! I will try to fully answer, but we have no debate limits and i'm open to proposals.

Something is missing here. An extensive contribution guide perhaps.

Yeah. I also add a link to the Community Maintainer Guidelines in the README.md, however it need to be defined.

Why linting rules aren't based on existing code style? I noticed you changed indentation of switch case blocks in code, rather than changing configuration of the linter. indent rule is configurable

It was mostly based on the existing code style (that was what i asked on init wizard). Some patterns was conflicting and some was set to default (like switch). It is ok to me to indent case "indent": ["error", 4, { "SwitchCase": 1 }].

I don't think linebreak-style rule should be enforced. For many users on Windows autocrlf option in git config is set to true. In fact I even think it's a default/recommended option. Maybe change it to a warning? I feel like that's going to be a pain for many Windows users when testing locally all though I'd like this rule to be enforced on CI's side.

Humm... yes... As i'm not a windows user i don't see this. A warning may be good to reduce linebreak mess. Today is not possible to develop Jimp on windows (with its own tools), however the changes on the test infra will help the windows users.

Would also love to know a reason for accepting arguments like (var { 0:_, 1:i, 2:X } = arguments;)

Ups! That was the remaining of previews idea, with a variable args size... Anyway. Thanks for pointing me this. It is totally unneeded now.

Linting of /tools/browser-build.js fails on no-console rule, You're probably aware of that, but that needs to be fixed before we could merge. Tetsts shouldn't fail from the start

No, no problem. tools are linted with some different rule, like tests.

"lint:extra": "eslint --rule '{no-console: \"off\"}' {*/,}{tools,examples}/*.js && echo Lint Extras Success"

It's not a ES6 code, why some var-s was replaced with let-s? Not all though.

That was my attempt to lint it with minimal change. The lint gave me a reasonable hint about some var redeclaration, mostly in for loops. Well... as we use babel for browser version, and babel simply translate let to var, noting changes there, however, on node we have less scope leak, and that can prevent weird problems. So, i change many var declarations to let.

Can JGD support be added with a wrapper? image/jgd is not a real MIME-type. I don't see much value in this for a regular user and it makes understanding of the main library's code little harder. Is it necessary to handle that as one of the image "formats"?

I could be a test wrapper, however i believe this has a potential to help Jimp users to add icons and other little images directly from their code. It replaces the need for unreadable base64 strings, for instance. As this is the only really new piece of code in the lib i put it in a separated module as we must make to everything after get the tests done. I try to not make it not to confuse the code, however a new code is ever more code. The main problem is on Jimp constructor with it's many ways to build itself, with different arg lists. This modularization will be my next target.

indentation was not corrected in some places 1, 2, 3

Interesting... I don't know why the lint don't point this. Thanks! If you know a missed rule, please tell me!

Is all my code changes good and enough? I believe not. But right now i believe (i|we) should do this way, and after getting tests done we must rewrite and modularize many pieces.

[thisconnect]

Great to see your tests! It is still possible to follow the discussion, but it is gettting harder. Would you mind discussing changes directly in a PR? As far as I know you can comment/discuss on each line of each file.

[rocketinventor]

Or, at least link to a specific line number.

[thisconnect]

So one question is if image/jgd should be officially added to Jimp or only in tests ENV?

[aurium]

I'm open to review my assumption, however i sincerely believe that can be useful for real world usage. For instance, see this script:

var _ = 0xFFFFFF00, i = 0x0044AA88, X = 0x0044AAFF;
var mapPointData = {
    width: 12, height: 12,
    data: [
        _,_,_,i,X,X,X,X,i,_,_,_,
        _,_,X,X,X,X,X,X,X,X,_,_,
        _,i,X,X,i,_,_,i,X,X,i,_,
        _,X,X,i,_,_,_,_,i,X,X,_,
        _,X,X,i,_,_,_,_,i,X,X,_,
        _,X,X,i,_,_,_,_,i,X,X,_,
        _,i,X,X,i,_,_,i,X,X,i,_,
        _,_,X,X,X,X,X,X,X,X,_,_,
        _,_,i,X,X,X,X,X,X,i,_,_,
        _,_,_,i,X,X,X,X,i,_,_,_,
        _,_,_,_,i,X,X,i,_,_,_,_,
        _,_,_,_,_,i,i,_,_,_,_,_
    ]
}

new Jimp(mapPointData, function(err, mapPoint) {
    if (err) throw err;
    Jimp.read("test/lenna.png", function (err, lenna) {
        if (err) throw err;
        lenna.composite(mapPoint, 200, 135).write("/tmp/lenna.jpg");
    });
});

I'm not saying that is cleaner script than one reading the same image from outside file, still, it opens new possibilities, faster experimentation scripting, bundleable images, and more in a readable fashion way.

It also adds the getJGD method, like the getBuffer, but something simpler to understand... As it helps to write tests, may help real world usage in some way too. I believe...

Well... is it ok to you?

[thisconnect]

I like it a lot for testing!

I have no opinion on including it in official api. Should it only be included in testing ENV or should it be officially included with documentation?

[iwsfg]

I still don't get why do we need all this window dressing. What you (@aurium) described in your last snippet looks a lot like regular {width: number, height: number, data: UInt8Array} object which is commonly used to represent decoded images. And which I suggested supporting as one of image sources in #203. Except in your case it's an array of pixels rather than separate RGBA values. I might be biased because it's my pull request, but I think we should support those instead as it is a more generic.

And if one needs image data exported as Uint32Array – they could just write a simple transformer.

function getJGDFromJimpImage (img) {
    const { width, height } = img.bitmap;
    const JGD = { width, height, data: new Uint32Array(width * height) };

    img.scan(0, 0, img.bitmap.width, img.bitmap.height, (x, y, idx) =>
        JGD.data[idx / 4] = img.bitmap.readUint32BE(idx, true));

    return JGD;
}

I don't think it should be part of the API of the main library in the way it's proposed. And I'm pretty skeptical about this argument of yours

however i believe this has a potential to help Jimp users to add icons and other little images directly from their code.

In barely complex example there will be much more different shades than 3, that keeping track of variable names will become problematic. I would rather let graphical editor do the job and export icon to PNG. And if I ever need to change its colors programmatically I see no problem in reading that file from disk and using blending modes to do so. Yes, it will be a little slower, but nice sub-pixel rendering matters more.

I tried to draw a simple marker icon out of curiosity, a small one, only 18x18 pixels. Switched to indexed colors and it said there's 32 different colors used. Textual representation just isn't meant for that. It's neat for testing purposes, yes, but end users wont need that.

[aurium]

Its is ok for me.

@Iwasawafag balance my arguments with some cons, and i trust his point of view. I'll steep back my proposal and i'll add JGD only as a testing wrapper.

About #203, i support that. And i think that we should modularize the Jimp constructor before. There are some ways to build this... and can be more!

[DanielRuf]

Tests and Travis are not activated so far.

[hipstersmoothie]

Working on getting circleci set up #465. have a PR that builds for 4 versions of node. Also to get around all the arguing about formatting and linting by adding prettier and xo as those have become great no config options for beautiful code. Make accessing PRs a lot easier when a style is enforced on commits.

once circleCI is running I'm closing this issue since testing is already in place.

[hipstersmoothie]

We run on GitHub Actions now 🎉