-
Notifications
You must be signed in to change notification settings - Fork 209
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
try moving to Github Actions from Travis for CI #1783
Conversation
Working from: Lines 1 to 49 in fc799bf
|
Looking good for a first attempt! We got all the way to gif tests and failed 2:
I wonder if these relate to setup - do we need... chromedriver... headless-gl... other stuff from cc @harshkhandeparkar @publiclab/is-maintainers just so you can see my progress here! Any help welcome! |
Thinking some of this stuff we may have to port over: Lines 35 to 49 in fc799bf
|
We need those dependencies for some of the modules.
If it works for non-GIFs, it should work for GIFs... |
OK we should be able to use this format to do this:
|
Co-authored-by: Barun Acharya <47106543+daemon1024@users.noreply.github.com>
OK, we got as far as Gif tests! |
It's the same as before, the same two tests were failing without the prerequisites too. |
I tried debugging it. It seems like the blur module is working fine but the generated base64 string is not matching the provided benchmark. Both seem to be blurred out when I converted them to image. What could be the reason behind it 🤔 |
It is possible that the two blurred images are very slightly off. This could happen even due to precision issues! We need to use a better way of comparing images. We used to use the |
That might be the case. Both image-sequencer/src/modules/Blur/Blur.js Line 51 in fc799bf
which convolutes the rgb channels. It is a gpu accelerated task, is it some limitation of the ci environment in github resulting in that precision issue? |
GPU precision can vary very slightly from device to device and in the case of modules like a blur, it won't even be noticed. We really need a looks-same alternative. |
Oh thanks for digging into this!
Could we try using this? https://github.com/mapbox/pixelmatch
Dunno if it would help...?
…On Sun, Jan 10, 2021, 9:55 PM Harsh Khandeparkar ***@***.***> wrote:
GPU precision can vary very slightly from device to device and in the case
of modules like a blur, it won't even be noticed. We really need a
looks-same alternative.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#1783 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAAF6JYVK67D7RL2WAR2Y7TSZJSDRANCNFSM4VNXUEIQ>
.
|
So I tried using https://github.com/rsmbl/Resemble.js since it looked more convenient to use than pixelmatch. I set the mismatch percentage to be less than 5% to pass the tests. Here's the CI run on my PR with resemble.js The gif tests seems to passing now tho it fails at the next step but it shouldn't be related to the scope of what we are trying to figure out right now. Update: Pixelmatch seems to need height and width of image as input, we would need to process the image for getting that information. Let me know if you know of a way to do that. |
This is awesome, @daemon1024 thank you! Pulling in your commits now. Let's see what's next. |
OK great - past Gif tests! Now:
note that |
OK, reproduced this in GitPod, full log copied here: https://gist.github.com/jywarren/eef2a7419c8a465842149554598e4045 |
Are Github actions faster or have higher caps? |
Github, NPM, Azure, those PRs to the Linux kernel, WSL, Github One.... It's all falling into place now 😂 |
I dunno about faster but travis was cut drastically so it has almost no
runs for open source projects so we needed an alternative. 😭
…On Tue, Jan 12, 2021, 2:48 PM Harsh Khandeparkar ***@***.***> wrote:
Github, NPM, Azure, those PRs to the Linux kernel, WSL, Github One....
It's all falling into place now 😂
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#1783 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAAF6J2B6PPXMZUABIGDKYDSZSRQ5ANCNFSM4VNXUEIQ>
.
|
Judging by what they are doing, they probably want to erase GitLab from the face of the planet. CI/CD, Web IDE, Collaboration... All of these were Gitlab's main features... |
For what it's worth, here is a complete log of running https://gist.github.com/jywarren/c1573f8ceac5ecb45db49c945a6d7115 It does seem related to |
Here's a note on using |
What do you both think? Can you offer a review? |
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.
🎉
Interesting. |
Yeah, seems like that was it -- i hope that by splitting out, we can see more clearly what is required for which job! |
Also noting this runs in 1/2 the time of Travis because it's parallel -- 2m 50s instead of 6m 26s in https://github.com/publiclab/image-sequencer/runs/1343663692 (for example) |
It works finally? 🥳 |
Yes, wanna look through? I think we can merge!
…On Mon, Jan 18, 2021, 9:59 PM Harsh Khandeparkar ***@***.***> wrote:
It works finally? 🥳
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#1783 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAAF6JZFLLPRR3G2HW6ADJ3S2TYPVANCNFSM4VNXUEIQ>
.
|
Let's do it in a follow-up, what do you think?
…On Mon, Jan 18, 2021, 10:48 PM Harsh Khandeparkar ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In .github/workflows/tests.yml
<#1783 (comment)>
:
> @@ -0,0 +1,209 @@
+name: tests
+on: [pull_request]
+jobs:
+ base-tests:
+ runs-on: ubuntu-18.04
+ steps:
+ - uses: ***@***.***
+ - name: Setup node
+ uses: ***@***.***
+ with:
+ node-version: '12'
Should we add v10 and v14 also? (At least v14 since it is the latest)
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#1783 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAAF6J4TF5WSVHLRBKROAQDS2T6JZANCNFSM4VNXUEIQ>
.
|
Sure, np! |
Hooray! Thank you both a TON for helping get this merged! I couldn't have done it without you! |
based on similar work in publiclab/plots2#8795 and publiclab/plots2#8793
In-depth discussion here: https://www.jeffgeerling.com/blog/2020/travis-cis-new-pricing-plan-threw-wrench-my-open-source-works