trezor / connect

:link: A platform for easy integration of Trezor into 3rd party services
Other
348 stars 262 forks source link

ci: Refactor GitHub Actions CI so Node v12, v14, v16 are tested #1076

Closed aloisklink closed 2 years ago

aloisklink commented 2 years ago

I noticed that your GitHub Actions CI didn't catch https://github.com/trezor/trezor-suite/issues/5107, so since I was already familiar with your CI (due to https://github.com/trezor/connect/pull/1050 (may cause merge conflicts)), I thought I'd fix it.

The issue with the previous GitHub Action is that actions/setup-node@v2 for Node v12 was only called in the setup job. Each job in GitHub CI is called on a different VM, so every other job defaulted to Node v16.

Although I could have just copied that setup action to every single job, to avoid code reuse, I've refactored all of the .github/workflows/test* files into:

Behavior changes:

Potentially fixes https://github.com/trezor/trezor-suite/issues/5111 (Karma tests and yarn build are only run on Node v12) (apologies for stealing something on your todo list, I had already almost completed this before I noticed you already had an issue for it!)

mroz22 commented 2 years ago

Hi, this contribution looks really nice. I need to get familiar with this approach a bit. Also sorry for not merging #1050 yet. It does not have that high priority at the moment, since we are going to move this project into trezor-sutie repository soon and all of this CI configuration will need some more engineering.

Also I would like to invite @vdovhanych to this.

aloisklink commented 2 years ago

I've upgraded the build and karma tests to use ubuntu-latest in https://github.com/trezor/connect/pull/1076/commits/86c0e578ca1989a857504638c1c5244f32cf0817, and it seems to work.

Also, there was a bug where the eth/btc/... tests were testing all the methods, which I've fixed in https://github.com/trezor/connect/pull/1076/commits/0123b020f7153226d2206a3fa26633b3bf3f5e9d (my fault, I had fixed this on my testing branch, but I think the git rebase/merge didn't apply it to this branch.

Also sorry for not merging https://github.com/trezor/connect/pull/1050 yet. It does not have that high priority at the moment, since we are going to move this project into trezor-sutie repository soon and all of this CI configuration will need some more engineering.

No worries :smile: CI changes are generally less important than other PRs!

mroz22 commented 2 years ago

Hello, I think we are ready for merge. Just please squash fixups and rebase.