FairChange icon indicating copy to clipboard operation
FairChange copied to clipboard

Create GitHub actions to test any code before merging

Open upkarlidder opened this issue 3 years ago • 9 comments

Background on the problem the feature will solve/improved user experience

Add GitHub Actions to test and build new code pushed to a branch or committed to a pull request. Code that passes all the steps in the GitHub action can be merged in the repository.

Describe the solution you'd like

A GitHub action runs whenever code is committed to any branch or PR is made to the main branch for each of the following components:

  • [ ] website folder
  • [ ] mobile folder
  • [ ] backend folder

Refer to a similar action that was implemented for the UI folder in Five Fifths Voter project.

Each action will

  • [ ] install dependencies
  • [ ] build for production
  • [ ] run all tests
  • [ ] linting with prettier. Discuss with the team on what linter to use.

Tasks

  • [ ] Ensure install, build and lint steps run locally on your system
  • [ ] Read more about GitHub actions
  • [ ] Create an action for each folder. You can read more JavaScript build actions here.
  • [ ] Commit and push your GitHub action code.
  • [ ] Go to the Actions tab to see if it worked.

Acceptance Criteria

GitHub action runs for every new code commit and PR.

FYI @demilolu

upkarlidder avatar Sep 26 '21 20:09 upkarlidder

Hi, I would like to work on this issue

ghost avatar Oct 04 '21 11:10 ghost

@arianahl go for it!

demilolu avatar Oct 04 '21 16:10 demilolu

@demilolu the pull request adds the git actions requested. A few things I would like to point out:

  1. backend and website don't have tests so I left that line commented out
  2. backend and mobile don't have lint configured so I left that line commented out
  3. website has lint but it is misconfigured and gives many errors when running

Are these known issues?

ghost avatar Oct 05 '21 19:10 ghost

@arianahl I think 1 and 2 are, but I'm not sure about 3.

@upkarlidder @Jamstew2 insight here?

demilolu avatar Oct 06 '21 23:10 demilolu

For 3, are you able to fix the errors @arianahl. That would be ideal.

upkarlidder avatar Oct 07 '21 00:10 upkarlidder

@upkarlidder If no one is working on the website part of the code I can give it go. Otherwise they would get a horrible merge on their hands. ~~Also a way to make it less intrusive is to change the current lint rule for parentheses: instead of having them on the next line leave them in the same line as most of the code already does.~~ Actually this is only a problem in the index.js script. The rest of the files do follow allman style. I can also go ahead and turn it on for the other two folders (different PR?).

ghost avatar Oct 07 '21 06:10 ghost

@upkarlidder If no one is working on the website part of the code I can give it go. Otherwise they would get a horrible merge on their hands. ~Also a way to make it less intrusive is to change the current lint rule for parentheses: instead of having them on the next line leave them in the same line as most of the code already does.~ Actually this is only a problem in the index.js script. The rest of the files do follow allman style. I can also go ahead and turn it on for the other two folders (different PR?).

It sounds good to me. @Jamstew2 and team can you please provide some guidance as well?

upkarlidder avatar Oct 07 '21 14:10 upkarlidder

@upkarlidder I fixed the linting issues as part of pull request #62 which adds to #61. I made sure the actions are still passing. Given the many changes I would recommend a thorough review from someone that knows the code just in case. I did test the website locally and I couldn't see any errors.

ghost avatar Oct 07 '21 20:10 ghost

@kyleni are you able to review the related PRs please?

demilolu avatar Nov 15 '21 20:11 demilolu