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 facilities for unit and functional tests #165

Merged
merged 1 commit into from
Nov 5, 2024

Conversation

danmcp
Copy link
Member

@danmcp danmcp commented Oct 31, 2024

There was already 1 unit test in place. The existing functional tests are being moved to scripts for consistency with other projects.

One new functional test has been added to test the mt_bench_branch generator as a starting example

I've included basic tests for mt_bench and mmlu. Which already found two issues:

  • mmlu_branch was using self.tasks instead of the number of results calculating the wrong average
  • Not specifying max_workers for mt_bench resulted in an error when it should have defaulted

@mergify mergify bot added CI/CD Affects CI/CD configuration documentation Improvements or additions to documentation testing Relates to testing ci-failure labels Oct 31, 2024
@mergify mergify bot added ci-failure and removed ci-failure labels Oct 31, 2024
@mergify mergify bot added ci-failure and removed ci-failure labels Oct 31, 2024
@mergify mergify bot removed the ci-failure label Oct 31, 2024
This was referenced Oct 31, 2024
@mergify mergify bot added the ci-failure label Oct 31, 2024
@mergify mergify bot removed the ci-failure label Oct 31, 2024
Copy link
Contributor

mergify bot commented Oct 31, 2024

This pull request has merge conflicts that must be resolved before it can be
merged. @danmcp please rebase it. https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/syncing-a-fork

@mergify mergify bot added the needs-rebase label Oct 31, 2024
@mergify mergify bot removed the needs-rebase label Oct 31, 2024
@danmcp danmcp force-pushed the unittests branch 2 times, most recently from 32b8217 to a5dc505 Compare October 31, 2024 22:57
Copy link
Member

@nathan-weinberg nathan-weinberg left a comment

Choose a reason for hiding this comment

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

Looked over all these changes and honestly everything LGTM - thank you for this @danmcp!

Two nits - one is in the comment, the other would be it'd be nice to add a badge for this new workflow similar to what is done for the CLI here: https://github.com/instructlab/instructlab/blob/main/README.md?plain=1#L4

.github/workflows/test.yml Show resolved Hide resolved
@danmcp
Copy link
Member Author

danmcp commented Nov 1, 2024

the other would be it'd be nice to add a badge for this new workflow similar to what is done for the CLI here: https://github.com/instructlab/instructlab/blob/main/README.md?plain=1#L4

Done!

There was one unit test already existing and this commit adds a couple of starter unit tests for mt_bench and mmlu.

Signed-off-by: Dan McPherson <[email protected]>
@mergify mergify bot merged commit 4c76340 into instructlab:main Nov 5, 2024
20 checks passed
@mergify mergify bot removed the one-approval label Nov 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI/CD Affects CI/CD configuration documentation Improvements or additions to documentation testing Relates to testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants