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

Add CI to this Repo #1

Merged
merged 12 commits into from
Jan 28, 2020
Merged

Add CI to this Repo #1

merged 12 commits into from
Jan 28, 2020

Conversation

ChihChengLiang
Copy link
Contributor

@ChihChengLiang ChihChengLiang commented Jan 27, 2020

Hi Wei Jie,

Notified that this is repo has no CI configured yet. Here are two possible CI options, Travis and Github Actions. Both are easy to configure but maybe one is enough, feel free to choose your favorite. Let me know and I'll remove the unused one to make the PR mergeable.

Copy link
Contributor Author

@ChihChengLiang ChihChengLiang left a comment

Choose a reason for hiding this comment

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

Explained some modifications to make the CI pass.

The CI pass here and here

@@ -1,3 +1,3 @@
[submodule "semaphore"]
path = semaphore
url = git@github.com:kobigurk/semaphore.git
url = https://github.com/kobigurk/semaphore.git
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Use https so that CI can pull this submodule

"/build/",
"/node_modules/",
"<rootDir>/build/",
"<rootDir>/node_modules/",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This change is required or the tests could not be resolved in Travis CI. The cause is that Travis CI put the project in a /home/travis/build/ directory, which will be ignored by Jest. The <rootDir> token is the standard way to avoid this issue.

@@ -7,7 +7,7 @@
"watch": "tsc --watch",
"build": "tsc",
"prepare": "npm run build",
"test": "./scripts/downloadSnarks.sh && jest",
"test": "./scripts/downloadSnarks.sh && jest --force-exit",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Use force-exit arg to silence Jest's complaint.

Jest did not exit one second after the test run has completed.

This usually means that there are asynchronous operations that weren't stopped in your tests. Consider running Jest with `--detectOpenHandles` to troubleshoot this issue.

@@ -30,7 +30,7 @@
},
"dependencies": {
"circomlib": "0.0.19",
"ethers": "4.0.37",
"ethers": "4.0.43",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

upgrade to fix this issue ethers-io/ethers.js#622

@weijiekoh
Copy link
Owner

Hi CC!

Thank you very much for this PR!

Having CI is very useful for this library and will help a lot with ensuring quality.

I have no preference between Travis or Github Actions. Feel free to pick whichever you think is best and let's merge this :)

Yours,

Wei Jie

@weijiekoh weijiekoh marked this pull request as ready for review January 28, 2020 02:57
@ChihChengLiang
Copy link
Contributor Author

Thanks for the review!

Then, let's do the cool kids' way. Give Github Actions a try.

The latest commit passes here.

@weijiekoh weijiekoh merged commit a622861 into weijiekoh:master Jan 28, 2020
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