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

Second working port to Rollup #525

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

Second working port to Rollup #525

wants to merge 6 commits into from

Conversation

robertleeplummerjr
Copy link
Member

I merged #499 too early, thinking that the node unit testing side was correctly implemented. Because of how drastic the change was, and that it no longer allowed testing in node, I reverted using git checkout HEAD~1 and a force push. I then reopened this in hopes that the changes will handle node eventually, and to keep a PR open and actionable.

@Symbitic
Copy link
Contributor

Tests should work in Node (I will double check). It just imports from the bundled commonjs file (in dist). And Node tests have to use require - that's why we import from the bundle instead of src.

For tests, I'm still looking into it. WebGL in browser is notoriously difficult to unit tests. It's even more difficult when there's hundreds of unit tests, like here. Plus, this is testing GPGPU math, not visual output to a canvas.

Some options include:

  • Puppeteer, either standalone or with Jest preset and/or environment.
  • Electron (mocha is often used to test WebGL this way)
  • TestCafe
  • Cypress

I'm trying to evaluate which one would be best.

P.S. is it too much to ask that v3 only exports the GPU class? I don't think many people use anything other than that.

@robertleeplummerjr
Copy link
Member Author

Tests should work in Node (I will double check).

All tests need to run in both Node and Browser.

WebGL in browser is notoriously difficult to unit tests.

We're aware, hence the seemingly obscure means of unit testing.

*Puppeteer, either standalone or with Jest preset and/or environment.
*Electron (mocha is often used to test WebGL this way)

Those (I've not checked the others) only test Chrome. For us, "The browser" is both desktop and mobile for Firefox, Chrome, Safari, and sometimes Edge.

P.S. is it too much to ask that v3 only exports the GPU class? I don't think many people use anything other than that.

Yes. There may be too much exposed in it as it is, but there still needs to be a baseline.

@robertleeplummerjr
Copy link
Member Author

If possible, is there any way we can not use building for node packages? It makes it so that we have double the src files. I mean, if it comes down to using import vs require(), to make us have to build the whole project just to run it in node, I'd say that isn't a good enough argument to use it.

@Symbitic
Copy link
Contributor

Symbitic commented Nov 7, 2019

You don't have to use double the src files. The QUnit browser tests already use the bundled version found in dist/ (all.html defines require() to simply redirect to an object based on the name).

Both node and browser tests should work.
To run the node tests:

yarn test test/features
yarn test test/internal
yarn test test/issues

(just like in the official instructions).

To run the browser tests: yarn watch
It starts rollup in watch mode, which will automatically start browsersync and open all.html in the web browser.

Both watch and test automatically run rollup to produce the dist/ bundle, so requiring the bundle to be built is invisible to the user.

I realize that Puppeteer and Electron use Chrome, but Babylon.js, THREE, A-Frame, and every other major WebGL project I know of have no problem with that; It lets them catch enough errors that the exceptional cases when other browsers differ can be handled manually. Though I admit, this one depends more on mathematical correctness than visual output.

@robertleeplummerjr
Copy link
Member Author

I'll resolve this: #521 then I'll work on helping here, as it is the last high priority item. Sorry to keep this help up. I did find an issue with build that I've fixed locally, I'll release today that may have contributed to the build issue I was seeing.

@Symbitic
Copy link
Contributor

Symbitic commented Nov 9, 2019

Thanks! I appreciate your interest.

Did you run the tests for both node and browser?

@robertleeplummerjr
Copy link
Member Author

Not yet.

@robertleeplummerjr
Copy link
Member Author

Is there any interest in continuing this effort? Quite a few conflicts.

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

Successfully merging this pull request may close these issues.

2 participants