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

Adding Backend testing to GitHub Actions. #99

Merged
merged 3 commits into from
Apr 16, 2024

Conversation

FernFinder
Copy link
Contributor

Overview

This pull request updates our GitHub Actions workflow to include testing for the backend.

Related Issue(s)

Closes #81.

Changes

The workflow file is updated to include a backend section now. This required some changes to the existing test in order to close the server connection after the tests are run. This must be done to prevent Jest from hanging. Comments have been added to assist with readability of code.

Changed

Workflow file and Users.test.js file.

Instructions for Testing

See GitHub Actions.

Checklist before Merging

  • I have performed a self-review of my own code.
  • I have added tests that prove my fix is effective or that my feature works.
  • I have added necessary documentation (if appropriate).
  • I have ensured the changes adhere to the project's coding standards and guidelines.

@FernFinder FernFinder changed the title Debugging GitHub Actions. Adding Backend testing to GitHub Actions. Apr 16, 2024
@FernFinder
Copy link
Contributor Author

It looks like there is a limitation with GitHub Actions. See this Article:
https://securitylab.github.com/research/github-actions-preventing-pwn-requests/

Here is a forum thread on the topic:
https://github.com/orgs/community/discussions/50161

The workflow does not have access to the main repo's secrets. This leads to a login error that fails the test. On my fork the tests are working just fine.
image

@FernFinder FernFinder changed the base branch from main to merging April 16, 2024 05:58
@FernFinder
Copy link
Contributor Author

FernFinder commented Apr 16, 2024

Just an idea I have. We could merge all fork pull requests to a branch, for example 'merging'. This could then trigger a pull_request_target job that would have access to secrets. Once that job is finished and succeeds it can be merged to main. We should test this tomorrow.

@thegreatzoidberg
Copy link
Contributor

Pull Request Review:

  • Readability:

    • New code is readable, well spaced-out, added comments.
  • Code Quality:

    • Nothing here looks bad to me.
  • Testing:

    • Workflow failed in the Backend Coverage Report step, but it looks like a small fix in another file.
    • Link for future reference
    • Already working on solution anyways from the pull request comments. Seems to be a secrets problem.
  • Functionality:

    • More testing coverage, specifically backend.
  • Housekeeping:

    • Don't see no junk files, don't see no AI mentioned.

Additional Comments:

  • Great work! Gonna need to open up a new Issue for current bug once this gets pulled.

Copy link
Contributor

@barkangel barkangel left a comment

Choose a reason for hiding this comment

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

Pull Request Review:

  • Readability:

    • Workflow code and user.tests.js are readable
  • Code Quality:

    • Code is adjusting previously created files that is easy to understand and general purpose is good.
  • Testing:

  • Workflow fail, however this is not being merged into main so it's OK and further note that it will be merged to a separate branch from main confirmed on Discord.

  • Functionality:

    • Modifies functionality of workflow, looks good.
  • Housekeeping:

    • No AI used/mentioned, looks good to me.

Additional Comments:

  • Nice job, PR approved for merging into new branch.

@FernFinder FernFinder marked this pull request as ready for review April 16, 2024 20:13
@FernFinder FernFinder merged commit 533b758 into UNLV-CS472-672:merging Apr 16, 2024
1 check failed
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.

Backend needs Github Actions to run tests for it
3 participants