-
Notifications
You must be signed in to change notification settings - Fork 7
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
Add GitHub CI #5
Conversation
- Use bash as pushd is not valid for sh - Also reset glslang.y - Use make -j (with number of cores)
- Use docker-compose for building and running
- Need to install jest before using it
Hmm looks like the jest test is failing on GitHub even though it works locally, maybe something with specific node.js version. https://github.com/pjoe/glslang.js/runs/525479759 Will take a look at converting to TypeScript tomorrow |
Since you're setting up tests, it would be useful to replicate the 3 test shaders from https://github.com/kainino0x/glslang.js/blob/master/tests/helper.mjs
I don't think there's any need to use typescript for a few small tests, probably easier if we don't add more complexity to the build. |
I saw the tests in helper.mjs, but ended up starting with something even simpler - just to get it working. My thinking was that after that I would try adding some of the tests from glslang itself: https://github.com/kainino0x/glslang/tree/851f3daad0efb7e69a6bec4fd07fe60067104a4a/Test. Maybe even validate the output too :) About TypeScript: as it's currently failing with some issue between jest and babel, I was merely thinking that this could be avoided by running the tests through ts (guess I'm also so used to it, that it's sort of second nature to me by now). Will try some more if I can get it working with just plain old js. |
After spending a bit more time investigating, seems like jest and mjs really are not good friends (yet): jestjs/jest#9430 At least I got it failing the same way locally, not sure how I had it working before :s Will give it a try with typescript, as this is something I know 'should work' tm :D |
Hmm after even more investigation: Something is wonky with the gslang build itself. When only building |
- Have to build node-devel twice to get right output (without import.meta) - Added tests from helper.mjs
Got it working now: https://github.com/pjoe/glslang.js/runs/528548315?check_suite_focus=true For some obscure reason have to run the node-devel build twice to get it to create the correct .js file without ESM promise loader using Also added the tests from helper.mjs properly setup with jest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tests look great. Do you want to debug the issue where you have to build twice or should we go ahead and merge? Happy to merge now and fix later, especially since we aren't doing frequent rereleases.
Let's merge first :) Would still like to figure out the build issue, but that can be done afterwards. I also suspect the issue is in glslang itself and not in this repo, but not sure. |
This adds initial CI using github actions.
Currently only web-min-nocompute and node-devel is build, but easy enough to add the other configs.
Also added a very basic test using jest that is run on each build. Will add better tests later :)