Skip to content

Conversation

@vincentfretin
Copy link
Contributor

This is based on #5091. I can rebase this PR once the first one is merged.
This PR removes travis and the old codecov uploader, replaces it by GitHub Actions.
You can see a successful pipeline here
https://github.com/vincentfretin/aframe/actions/runs/2868225958

@vincentfretin vincentfretin mentioned this pull request Aug 16, 2022
@vincentfretin
Copy link
Contributor Author

Note that package-lock.json needs to be in the repo for the GitHub Actions npm cache to properly work. It will reuse the npm cache if package-lock.json didn't change.

codecov upload should work, but I can't test it on my fork. It finds the tests/coverage/lcov.info file and try to upload it to codecov, but I get a 404 there.

I set up testing only on chrome, but you can enable it for firefox if you want, it's just commented.

@vincentfretin
Copy link
Contributor Author

I enabled the tests on firefox, it seems fine. You'll see if there are some tests that are randomly failing on Firefox or not later on other PRs. If there is an issue with it, just comment it.
The CI is actually already running on this PR. \o/
https://github.com/aframevr/aframe/runs/7859579555?check_suite_focus=true

You can merge this PR and close the first one, or merge the first one, then I rebase this one and you merge, as you wish.

@vincentfretin
Copy link
Contributor Author

Compared to the previous travis config, I added a check for the minified build that runs npm run dist.
In travis, TEST_SUITE=unit was set but this variable is not used in any part of the code, I think it was to parallelize the tests but from travis documentation I think it was misused or didn't have an effect at all. Anyway I didn't use it in the new CI.

@vincentfretin
Copy link
Contributor Author

codecov upload is working, it uploaded the results from this PR:
https://codecov.io/github/aframevr/aframe/commit/057863548eea0358f0b1a0307a1fb67bcd4c4f18

karma.conf.js
node_modules
npm-debug.log*
package-lock.json
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this intentional?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dmarcos
Copy link
Member

dmarcos commented Aug 17, 2022

It needs rebase!

@vincentfretin
Copy link
Contributor Author

I rebased.

@vincentfretin
Copy link
Contributor Author

About putting package-lock.json in the repo, mainly to have the npm cache in CI, it may add a burden to you to update the file from time to time to keep up to date dependencies for patch versions of some of the dependencies.
Having the .npm folder cached in CI means reducing the "Install dependencies" step from 2m51 to 1m18.
I can remove it.

@vincentfretin
Copy link
Contributor Author

done, good to merge

@dmarcos
Copy link
Member

dmarcos commented Aug 17, 2022

Thanks so much! This is great work. Tests don't get much love. I let them rotten a little bit 😄

@dmarcos dmarcos merged commit bacaabd into aframevr:master Aug 17, 2022
@vincentfretin vincentfretin deleted the setup-ci branch August 17, 2022 15:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

2 participants