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 unit tests workflow #865

Merged

Conversation

drikusroor
Copy link
Contributor

@drikusroor drikusroor commented Apr 11, 2023

Background

The last days we have seen quite some instability after some PRs had been merged, related to config properties, json parsing, infinite loops. This PR doesn't necessarily fix those issues but unit tests run in a workflow and is an attempt to try to set a first baseline for protecting the quality and desired functionality of the code. Additionally, it allows other developers to add new or change existing unit tests in a relatively straight-forward way.

Changes

  • Add a workflow that runs the unit tests after every commit on the master branch and for every PR targeted towards the master branch.
  • Code coverage is also generated, but I haven't made a decision on how to report those (in a badge?). Perhaps that something for the owner(s) of this repository to decide.
  • Fix the module imports for the existing unit tests.
  • Fix some of the json_parser unit tests.
  • Add some unit tests for the Config class.
  • Fix the cfg.debug issue that was already reported earlier. These changes will probably "disappear" from the changed files once someone else's PR fixes it. (See also here, here, here, and here 😅 )

Documentation

  • Pipeline status is documented in a badge in the beginning of README.md
  • How to run the unit tests and the coverage variant is documented at the end of README.md

Test Plan

The test plan is to create this PR and see if:

  • The unit tests are running and succeeding
  • The pipeline status badge is visible and working in README.md

PR Quality Checklist

  • My pull request is atomic and focuses on a single change.
  • I have thouroughly tested my changes with multiple different prompts. (N/A)
  • I have considered potential risks and mitigations for my changes.
  • I have documented my changes clearly and comprehensively.
  • I have not snuck in any "extra" small tweaks changes => Well, the cfg.debug issue was "snuck in". Please let me know if you want me to remove it.

richbeales
richbeales previously approved these changes Apr 12, 2023
@richbeales
Copy link
Contributor

Think we should add flake8 too

@drikusroor
Copy link
Contributor Author

Think we should add flake8 too

Do you want me to do that in this PR or in a separate PR after this one has been merged?

@richbeales
Copy link
Contributor

If you're happy to, it'd be great to add it in a separate PR.

@MarjovanLier
Copy link

Let's go :)

@drikusroor drikusroor mentioned this pull request Apr 12, 2023
5 tasks
@drikusroor
Copy link
Contributor Author

If you're happy to, it'd be great to add it in a separate PR.

I've added it in #923, but it's dependent on this PR since the whole workflow is added here. I think it'd be a good idea to first merge this (#865) PR and then the flake8 PR (#923).

@drikusroor drikusroor force-pushed the add-unit-tests-workflow branch from a5ca5ee to 5265d81 Compare April 12, 2023 08:16
@richbeales
Copy link
Contributor

@Torantulino would be good to get your eyes over this, and get this and 923 merged if you're happy?

richbeales
richbeales previously approved these changes Apr 12, 2023
@richbeales richbeales merged commit 0270696 into Significant-Gravitas:master Apr 12, 2023
sindlinger pushed a commit to Orgsindlinger/Auto-GPT-WebUI that referenced this pull request Sep 25, 2024
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.

3 participants