-
Notifications
You must be signed in to change notification settings - Fork 51
[WIP] vitest evals #1232
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
base: main
Are you sure you want to change the base?
[WIP] vitest evals #1232
Conversation
ibolmo
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
various questions
|
|
||
| // indicate the project name the tests will be sent to | ||
| const bt = wrapVitest( | ||
| { test, expect, describe, afterAll }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd recommend for users to:
import * as vitest from 'vitest';
const { describe, expect, test, ... } = wrapVitest(vitest);
simpler, less prone to error, and future proof if we add more functions to our support
| metadata: { category: "math" }, | ||
| tags: ["arithmetic"], | ||
| }, | ||
| async ({ input, expected }) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
til.. so the context is re-inserted as an argument
| afterAll: vitestMethods.afterAll || (() => {}), | ||
| beforeEach: vitestMethods.beforeEach, | ||
| afterEach: vitestMethods.afterEach, | ||
| logOutputs: (outputs: Record<string, unknown>) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
didn't expect this method 🤔
| score: number; | ||
| metadata?: Record<string, unknown>; | ||
| }) => { | ||
| const span = currentSpan(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what if instead of these methods we just asked folks to use currentSpan?
| @@ -0,0 +1,46 @@ | |||
| import tsconfigPaths from "vite-tsconfig-paths"; | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
didn't expect to see this file in here 🤔
| return originalDescribe(suiteName, () => { | ||
| // Lazily initialize experiment context on first access | ||
| let context: ExperimentContext | null = null; | ||
| const getOrCreateContext = (): ExperimentContext => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
like we want to extract and use the same getOrCreate inside of test() calls
| "Braintrust: vitestMethods.describe is required. Please pass in the describe function from vitest.", | ||
| ); | ||
| } | ||
| if (!vitestMethods.expect) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i didn't see a wrapExpect in wrapper.ts. I wonder if each expect is a scorer?
| }); | ||
|
|
||
| // If test function returns a value, log it as output | ||
| if (testResult !== undefined) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think if you traced(maybeFn || configOrFn, ...) you may have gotten this automatically?
| scores: { | ||
| pass: 0, | ||
| }, | ||
| metadata: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you should probably just throw again. the traced() call should handle the error.
| datasetExamples: Map<string, string>; // test name -> example id | ||
| } | ||
|
|
||
| // Global context holder (one per describe block) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what happens with concurrent calls i.e.
it.concurrent(
describe(..., () => {
})
);
it.concurrent(
describe(..., () => {
})
);
did you give currentExperiment a try?
No description provided.