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

fixing various error occuring during testing (some related to window keyword, some gl-context/puppeteer) #1285

Closed
keshav234156 opened this issue Oct 28, 2019 · 25 comments

Comments

@keshav234156
Copy link
Member

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

@keshav234156
Copy link
Member Author

keshav234156 commented Oct 28, 2019

cc: @jywarren @vaibhavmatta

@keshav234156
Copy link
Member Author

keshav234156 commented Oct 28, 2019

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

@jywarren
Copy link
Member

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!

@keshav234156
Copy link
Member Author

keshav234156 commented Oct 28, 2019

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

@jywarren
Copy link
Member

jywarren commented Oct 28, 2019 via email

@keshav234156
Copy link
Member Author

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

@jywarren
Copy link
Member

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

@vaibhavmatta
Copy link

@jywarren I was about to suggest that!

@keshav234156
Copy link
Member Author

@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

@jywarren
Copy link
Member

jywarren commented Nov 2, 2019

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!!!!

@keshav234156
Copy link
Member Author

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

@jywarren
Copy link
Member

jywarren commented Nov 4, 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'?

@keshav234156
Copy link
Member Author

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

@jywarren
Copy link
Member

jywarren commented Dec 2, 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?

if (typeof window !== 'undefined') { isBrowser = true; }

@keshav234156
Copy link
Member Author

keshav234156 commented Dec 3, 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?

@jywarren
Copy link
Member

jywarren commented Dec 3, 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
Copy link
Member

jywarren commented Dec 3, 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!?

@keshav234156
Copy link
Member Author

keshav234156 commented Dec 3, 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

@jywarren
Copy link
Member

jywarren commented Dec 3, 2019 via email

@keshav234156
Copy link
Member Author

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

@SidharthBansal
Copy link
Member

I think you guys will teach me IS this GCI.

@SidharthBansal
Copy link
Member

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

@jywarren
Copy link
Member

jywarren commented Dec 10, 2019 via email

@keshav234156
Copy link
Member Author

#1376 #1377 #1378

@jywarren jywarren changed the title fixing various error occuring during testing fixing various error occuring during testing (some related to window keyword, some gl-context/puppeteer) Jul 12, 2020
@jywarren
Copy link
Member

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

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

No branches or pull requests

5 participants