added run dev server action to pull request workflow #1371
Conversation
Our Pull Request Approval ProcessWe have these basic policies to make the approval process smoother for our volunteer team. Testing Your CodePlease make sure your code passes all tests. Our test code coverage system will fail if these conditions occur:
The process helps maintain the overall reliability of the code base and is a prerequisite for getting your PR approved. Assigned reviewers regularly review the PR queue and tend to focus on PRs that are passing. ReviewersWhen your PR has been assigned reviewers contact them to get your code reviewed and approved via:
Reviewing Your CodeYour reviewer(s) will have the following roles:
CONTRIBUTING.mdRead our CONTRIBUTING.md file. Most importantly:
Other
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Congratulations on making your first PR! 🎊 If you haven't already, check out our Contributing Guidelines and PR Reporting Guidelines to ensure that you are following our guidelines for contributing and creating PR.
|
I have added a job that runs the dev server after testing is finished. Is there anything that I am missing? if so, let me know and i will make the required changes |
Codecov Report
@@ Coverage Diff @@
## develop #1371 +/- ##
========================================
Coverage 98.17% 98.17%
========================================
Files 184 184
Lines 10767 10767
Branches 835 835
========================================
Hits 10571 10571
Misses 186 186
Partials 10 10 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
|
Can i get some help with the checks not passing? @palisadoes
|
|
|
I have setup up a separate job for running dev server with dummy variables that worked on local setup, so i am assuming it should work on the workflow as well. let me know if i need to change anything |
.github/workflows/pull-request.yml
Outdated
| - name: Sleep for 10s | ||
| uses: juliangruber/sleep-action@v1 | ||
| with: | ||
| time: 10s | ||
|
|
There was a problem hiding this comment.
why we need to wait for 10 seconds, is there anything that will break without it ? @ishtails
There was a problem hiding this comment.
I don't believe we need to wait, but this sleep command was there in the test job as well before running tests, so I put it here to be on the safe side, as I felt it might be there for some specific reason...
There was a problem hiding this comment.
@ishtails github actions steps are run in series are they not? So idk what we're waiting 10 seconds for.
Anyway a clean way to test whether server successfully starts or stops is listening for node.js events. The useful events would be the listening, close and error events. You would listen to these events to determine if server successfully starts, stops or errors out in between the process.
This can be done by importing the server instance in a test file created for this test case and checking for those events. When and where you choose to run this particular test case is upto you to decide.
There was a problem hiding this comment.
I will remove the sleep command from the job then, I also believe it is not required.
That is a good idea, to write a test for it. However I don't have any experience writing tests as of now. Can we have it as a separate issue maybe? So that either someone else could write the test for it, or I could just do it in the future.
There was a problem hiding this comment.
@ishtails It seems there are some oddities about github actions that I wasn't aware about. The 10 second wait seems to be done for the server to successfully start because the workflow doesn't wait for the server to successfully start I guess? Or the the npm run dev gives the successful status code without it actually being successful, for example doing things asynchronously. Something like that.
Also an action just to check if the server successfully starts seems wasteful, it'd be better to have this as a part of some end to end test workflow. For example this action https://github.com/PalisadoesFoundation/talawa-api/blob/develop/.github/workflows/push-documentation.yml already makes use of npm run dev. Starting the server itself doesn't tell us anything about whether all the services of the server are working perfectly.
There was a problem hiding this comment.
I am unaware of any such issues. Let me know if I should put the sleep command there or not...
If someone could explain the need for it, it would be really nice.
The issue originally just stated that there should be a job to check if the dev server starts or not, shouldn't the check passing or failing mean that the server started successfully or failed to start anyway? If not, How can I modify it to verify the status of services running in here?
|
@xoldyckk @noman2002 We created the issue because there were times when the PR tests passed, but the application would not start. Some contributors would make modifications to the code to to satisfy the code coverage requirements, but ignored the usability. This is to help prevent such short sighted work. |
palisadoes
merged commit 31f7a3a
into
PalisadoesFoundation:develop
This reverts commit 31f7a3a.
|
This has to be reverted. There is a syntax error. |
What kind of change does this PR introduce?
PR CI/CD workflow feature
Issue Number:
Fixes #1343
Did you add tests for your changes?
No.
Snapshots/Videos:
N/A
If relevant, did you update the documentation?
No
Summary
Added a final github action to run the dev server using "npm run dev" before finishing checks as suggested by issue #1343
Does this PR introduce a breaking change?
No
Other information
NA
Have you read the contributing guide?
Yes