Image-sequencer: fixing various error occuring during testing (some related to window keyword, some gl-context/puppeteer)

Created on 28 Oct 2019  ·  25Comments  ·  Source: publiclab/image-sequencer

A lot errors(reference errors,type errors,Unhandled 'error' events,WebAssembly acceleration error) could be seen in travis logs.this could be seen in https://travis-ci.org/publiclab/image-sequencer/jobs/595161585?utm_medium=notification&utm_source=github_status .These errors our preventing us to completly test our module ie all the individual modules are not being tested.Some of these errors have occured after merging PR #1234 (as all the individual modules were tested before this in #1233 travis logs. )

discussion help wanted high-priority testing
Source
picture keshav234156
❤2 👍2

Most helpful comment

@jywarren So the problem is with PR #1090. After reverting the changes of #1090(as done in #1289) the modules that are preventing all modules to being tested are:

1.fisheye-gl
2.text-overlay.
3.webgl-distort.

only after soving errors related to the above mentioned modules we would be able to test are modules completely.

Next we should try to fix the errors that are found in modules.These modules include :
1.colorbar
2.draw-rectangle
3.gamma-correction
4.paint-bucket
5.replace-color
6.rotate module
7.tint module

picture keshav234156 on 2 Nov 2019
🚀1 ❤1 🎉1 👍1

All 25 comments

cc: @jywarren @vaibhavmatta

keshav234156 picture keshav234156 on 28 Oct 2019

@jywarren we have faced similar issue as mentioned in #659 #1113.

keshav234156 picture keshav234156 on 28 Oct 2019
👍1

Is there anything you all can see particularly linking the mixin-deep module that was upgraded in #1234 to the errors not triggering a failed build, do you think?

Also cc'ing @aashna27 @HarshKhandeparkar @harshithpabbati @tech4GT @Divy123 in case you all have insights here. Thanks! Testing and stabilizing our tests will do a huge amount to strengthen the core of our code!

jywarren picture jywarren on 28 Oct 2019

right now No, i think it's just because of upgrade in version of mixing-deep

keshav234156 picture keshav234156 on 28 Oct 2019

We could test this by opening a PR rolling back the version and seeing if
the tests run correctly?

On Mon, Oct 28, 2019, 3:48 PM keshav234156 notifications@github.com wrote:

No i think it's because of upgrade in version of mixing-deep

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/publiclab/image-sequencer/issues/1285?email_source=notifications&email_token=AAAF6J26E63IKLQKDP3LMF3QQ46VLA5CNFSM4JF6SB22YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOECOFEQA#issuecomment-547115584,
or unsubscribe
https://github.com/notifications/unsubscribe-auth/AAAF6J236AYAXCLQ62NJX6TQQ46VLANCNFSM4JF6SB2Q
.

jywarren picture jywarren on 28 Oct 2019

@jywarren reverting the changes of #1234 in #1286 didn't work.

keshav234156 picture keshav234156 on 28 Oct 2019

Hmm. is the next step to try to get the failing tests to cause Travis to return a failure?

jywarren picture jywarren on 29 Oct 2019

@jywarren I was about to suggest that!

vaibhavmatta picture vaibhavmatta on 29 Oct 2019
🎉1 👍1

@jywarren So the problem is with PR #1090. After reverting the changes of #1090(as done in #1289) the modules that are preventing all modules to being tested are:

1.fisheye-gl
2.text-overlay.
3.webgl-distort.

only after soving errors related to the above mentioned modules we would be able to test are modules completely.

Next we should try to fix the errors that are found in modules.These modules include :
1.colorbar
2.draw-rectangle
3.gamma-correction
4.paint-bucket
5.replace-color
6.rotate module
7.tint module

keshav234156 picture keshav234156 on 2 Nov 2019
🚀1 ❤1 🎉1 👍1

Oh wow! So, wait, can you clarify... const $ = window.$; caused the error. When we remove that, these modules then error out?

1.fisheye-gl
2.text-overlay.
3.webgl-distort.

  1. Do they need this $ const defined? We could potentially run that line only if we're in a specific environment where window is defined, maybe?
  2. Then, are you saying, more errors have accumulated in these other modules since then, and we can circle back in and resolve those?
  3. Finally, should we have trusted the change in coverage reported there, and rejected that PR?

Thank you so much for this excellent detective work!!!!

jywarren picture jywarren on 2 Nov 2019

@jywarren
1.You are right window is only defined in browser environment but is not defined in node.js environment. so they causing errors.similar errors could be seen with fisheye-gl,text-overlay,webgl-distort so we have fix it as well
2.yes,many errors have been accumulated since then as are modules where not being test.these include 7 modules that i have listed above.
3.we should have trusted the coverage report as coverage was decreased by a large amount with the PR.

keshav234156 picture keshav234156 on 2 Nov 2019

Uh ok - so i believe there is a way we can see if we're in a browser environment, but i'm a little rusty. It could be either options.inBrowser or typeof window == 'undefined'?

jywarren picture jywarren on 4 Nov 2019

@jywarren after merging #1289 , our next step should be to solve the error as given in the below error logs.Do you have any idea how can we solve this?

/home/keshav/image-sequencer/node_modules/fisheyegl/src/fisheyegl.js:304
  window.FisheyeGl = FisheyeGl;
  ^

ReferenceError: window is not defined
    at Object.<anonymous> (/home/keshav/image-sequencer/node_modules/fisheyegl/src/fisheyegl.js:304:3)
    at Module._compile (internal/modules/cjs/loader.js:689:30)
    at Module.replacementCompile (/home/keshav/image-sequencer/node_modules/append-transform/index.js:58:13)
    at Module._extensions..js (internal/modules/cjs/loader.js:700:10)
    at Object.<anonymous> (/home/keshav/image-sequencer/node_modules/append-transform/index.js:62:4)
    at Module.load (internal/modules/cjs/loader.js:599:32)
    at tryModuleLoad (internal/modules/cjs/loader.js:538:12)
    at Function.Module._load (internal/modules/cjs/loader.js:530:3)
    at Module.require (internal/modules/cjs/loader.js:637:17)
    at require (internal/modules/cjs/helpers.js:22:18)
    at Array.DoNothing (/home/keshav/image-sequencer/src/modules/FisheyeGl/Module.js:1:5010)
    at insertStep (/home/keshav/image-sequencer/src/InsertStep.js:1:8350)
    at InsertStep (/home/keshav/image-sequencer/src/InsertStep.js:1:8500)
    at AddStep (/home/keshav/image-sequencer/src/AddStep.js:1:1000)
    at Object.addSteps (/home/keshav/image-sequencer/src/ImageSequencer.js:1:27737)
    at Test.t (/home/keshav/image-sequencer/test/core/templates/module-test.js:23:15)
keshav234156 picture keshav234156 on 30 Nov 2019

This last error happens because it's designed for use in a browser -- which has the variable window. But in nodejs this doesn't exist. I'm not sure, i think we can use this syntax to skip this when in node?

https://github.com/publiclab/image-sequencer/blob/f1c94fd78850f6558090697ef4f30f55bdd0f236/src/ImageSequencer.js#L1

jywarren picture jywarren on 2 Dec 2019

@jywarren ok,I added the following the lines of code in the test file 

const { JSDOM } = require('jsdom');
var { window } = new JSDOM();

adding this error logs where

fisheye-gl module loads correctly

✔ fisheye-gl module is getting loaded

fisheye-gl module loads with correct options

✔ Option a is correct
✔ Option b is correct
✔ Option Fx is correct
✔ Option Fy is correct
✔ Option scale is correct
✔ Option x is correct
✔ Option y is correct

fisheye-gl module works correctly

(node:25070) UnhandledPromiseRejectionWarning: Error: Evaluation failed: ReferenceError: __cov_7dJRRx77$AD_iFr6aR7E3A is not defined
    at __puppeteer_evaluation_script__:1:12
    at ExecutionContext._evaluateInternal (/home/keshav/image-sequencer/node_modules/puppeteer/lib/ExecutionContext.js:122:13)
    at process._tickCallback (internal/process/next_tick.js:68:7)
  -- ASYNC --
    at ExecutionContext.<anonymous> (/home/keshav/image-sequencer/node_modules/puppeteer/lib/helper.js:111:15)
    at DOMWorld.evaluate (/home/keshav/image-sequencer/node_modules/puppeteer/lib/DOMWorld.js:112:20)
    at process._tickCallback (internal/process/next_tick.js:68:7)
  -- ASYNC --
    at Frame.<anonymous> (/home/keshav/image-sequencer/node_modules/puppeteer/lib/helper.js:111:15)
    at Page.evaluate (/home/keshav/image-sequencer/node_modules/puppeteer/lib/Page.js:827:43)
    at Page.<anonymous> (/home/keshav/image-sequencer/node_modules/puppeteer/lib/helper.js:112:23)
    at page.addScriptTag.then (/home/keshav/image-sequencer/src/modules/_nomodule/gl-context.js:9:992)
    at process._tickCallback (internal/process/next_tick.js:68:7)
(node:25070) UnhandledPromiseRejectionWarning: Unhandled promise rejection. This error originated either by throwing inside of an async function without a catch block, or by rejecting a promise which was not handled with .catch(). (rejection id: 1)

So now we can test that the module loads correctly with the correct option. Do you know, why the error is occurring in the works correctly part?

keshav234156 picture keshav234156 on 3 Dec 2019

This is some great progress!!! Awesome!!! I don't know what this error is about, but maybe @Divy123 or @HarshKhandeparkar or others from @publiclab/is-reviewers or @publiclab/image-sequencer-guides knows?

jywarren picture jywarren on 3 Dec 2019

Perhaps we should merge what we have so far here, and work on the remainder in a fresh issue? To build on the great progress you've made so far!?

jywarren picture jywarren on 3 Dec 2019

Ok, found that error is occurring as we are using gl-context which in turn is using puppeteer. It is a Code Transpilation Issues ie If you are using a JavaScript transpiler like babel or TypeScript, calling evaluate() with an async function might not work. This is because while puppeteer uses Function.prototype.toString() to serialize functions while transpilers could be changing the output code in such a way it's incompatible with the puppeteer.

also before the PR #1007, we were not using gl-context, so the test worked for them

keshav234156 picture keshav234156 on 3 Dec 2019

whoa, excellent investigating!

On Tue, Dec 3, 2019 at 4:07 PM keshav234156 notifications@github.com
wrote:

Ok, found that error is occurring as we gl-context which in turn is using
puppeteer. It is a Code Transpilation Issues ie If you are using a
JavaScript transpiler like babel or TypeScript, calling evaluate() with an
async function might not work. This is because while puppeteer uses
Function.prototype.toString() to serialize functions while transpilers
could be changing the output code in such a way it's incompatible with the
puppeteer.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/publiclab/image-sequencer/issues/1285?email_source=notifications&email_token=AAAF6JYZYKUEAKY7B2MKDZTQW3C6RA5CNFSM4JF6SB22YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEF22BUI#issuecomment-561357009,
or unsubscribe
https://github.com/notifications/unsubscribe-auth/AAAF6J3K5A6WCAN4XE7XRZDQW3C6RANCNFSM4JF6SB2Q
.

jywarren picture jywarren on 3 Dec 2019

our next step should be fix 7 modules.For that, i found the module was changed as it has some bugs like the rotate module was earlier having fixed dimension but corresponding benchmarks were not changed as a result they are failing. So for this, we can change the benchmarks of those modules. Then our next step should be if there is any failure in the test then Travis should raise an error rather than passing it

keshav234156 picture keshav234156 on 10 Dec 2019
🎉1

I think you guys will teach me IS this GCI.

SidharthBansal picture SidharthBansal on 10 Dec 2019
❤3

My inbox has more IS emails than other repos and I am reviewing some PRs too. :-) 🚀

SidharthBansal picture SidharthBansal on 10 Dec 2019
🚀2

:-) thanks for being on top of this, Sidharth!!!!

On Tue, Dec 10, 2019 at 1:27 PM Sidharth Bansal notifications@github.com
wrote:

My inbox has more IS emails than other repos and I am reviewing some PRs
too. :-) 🚀

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/publiclab/image-sequencer/issues/1285?email_source=notifications&email_token=AAAF6J4WAGOJ2RYGVFMNAYLQX7NPNA5CNFSM4JF6SB22YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEGQIKWQ#issuecomment-564168026,
or unsubscribe
https://github.com/notifications/unsubscribe-auth/AAAF6J2P44DTP3S2OGYL6LDQX7NPNANCNFSM4JF6SB2Q
.

jywarren picture jywarren on 10 Dec 2019

1376 #1377 #1378

keshav234156 picture keshav234156 on 19 Dec 2019

I'm closing this in favor of the now narrowed #1378! Thanks!

jywarren picture jywarren on 12 Jul 2020
Was this page helpful?
0 / 5 - 0 ratings

Related issues

jywarren picture jywarren  ·  5Comments
kindanduseful picture kindanduseful  ·  5Comments
harshkhandeparkar picture harshkhandeparkar  ·  3Comments
harshkhandeparkar picture harshkhandeparkar  ·  4Comments
keshav234156 picture keshav234156  ·  4Comments