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 --elide-lines override flag for workspace filtering #15837

Merged
merged 5 commits into from
Dec 22, 2024

Conversation

martinamps
Copy link
Contributor

@martinamps martinamps commented Dec 18, 2024

What does this PR do?

This adds a new flag --elide-lines to bun test to allow overriding the default value of 10. Lots of other folks ran into this in #12431

How did you verify your code works?

Manually and via unit tests

before:

✗ bun run  --filter gateway test
gateway test $ bun test
│ [28 lines elided]
│
│ tests/unit/openai/structuredOutput.test.ts:
│ (pass) OpenAI Structured Output > should handle structured output for research paper extraction [0.80ms]
│ (pass) OpenAI Structured Output > should handle structured output for math reasoning [0.28ms]
│ (pass) OpenAI Structured Output > should handle invalid structured output [0.29ms]
│
│  22 pass
│  0 fail
│  155 expect() calls
│ Ran 22 tests across 4 files. [66.00ms]
└─ Done in 77 ms

after:

✗ /Users/martinamps/code/bun/build/release/bun run --elide-lines 0 --filter gateway test
gateway test $ bun test
│ bun test v1.1.39 (1d485617)
│
│ tests/unit/model-definitions/getDefinition.test.ts:
│ (pass) Model Definitions > should return correct OpenAI chat model definition
│ (pass) Model Definitions > should return correct Anthropic chat model definition [0.14ms]
│ (pass) Model Definitions > should handle unknown model gracefully
│ (pass) Model Definitions > should type check different model types correctly [0.03ms]
│ (pass) Model Definitions > provider fields should be present and correct type [0.04ms]
│
│ tests/unit/openai/functionCalling.test.ts:
│ Cache Middleware is not enabled because caches is not defined.
│ (pass) OpenAI Function Calling > should handle function calling with a single function [3.81ms]
│ (pass) OpenAI Function Calling > should handle function calling with multiple functions [0.67ms]
│ (pass) OpenAI Function Calling > should handle function calling with a specified function [0.53ms]
│ (pass) OpenAI Function Calling > should handle function execution and follow-up [0.53ms]
│ (pass) OpenAI Function Calling > should handle errors in function calling [0.33ms]
│ (pass) OpenAI Function Calling > should handle function calling with invalid arguments [0.46ms]
│ (pass) OpenAI Function Calling > should handle streaming responses [1.75ms]
│
│ tests/unit/openai/chatCompletions.test.ts:
│ (pass) OpenAI Chat Completions > should forward a basic chat completion request [0.49ms]
│ (pass) OpenAI Chat Completions > should handle rate limiting [0.25ms]
│ mamps here3
│ (pass) OpenAI Chat Completions > should handle network errors [0.33ms]
│ (pass) OpenAI Chat Completions > should reject requests with invalid content type [0.21ms]
│ (pass) OpenAI Chat Completions > should use the correct API key [0.28ms]
│ (pass) OpenAI Chat Completions > should handle very large requests [0.32ms]
│ (pass) OpenAI Chat Completions > should handle streaming responses [0.36ms]
│
│ tests/unit/openai/structuredOutput.test.ts:
│ (pass) OpenAI Structured Output > should handle structured output for research paper extraction [0.87ms]
│ (pass) OpenAI Structured Output > should handle structured output for math reasoning [0.28ms]
│ (pass) OpenAI Structured Output > should handle invalid structured output [0.31ms]
│
│  22 pass
│  0 fail
│  155 expect() calls
│ Ran 22 tests across 4 files. [68.00ms]
└─ Done in 77 ms

The coupling of elide to pretty output was a little tight so my matching for the output is a little relaxed as a result and I had to make minor tweaks to the test runner to pass in the extra env flags

@jakeboone02
Copy link
Contributor

Might be nice to have this as a bunfig option as well, so it doesn't have to be included in every command.

@Jarred-Sumner
Copy link
Collaborator

Nice. Looks like there's some test failures, but this is a good idea and I would like to merge it once the tests pass.

src/cli.zig Outdated Show resolved Hide resolved
@martinamps
Copy link
Contributor Author

@jakeboone02 sounds reasonable! I attempted to add it here https://github.com/oven-sh/bun/blob/main/src/bunfig.zig#L592 and it didn't do anything - happen to know why off the top of your head before I debug?

@Jarred-Sumner awesome 🙏 which tests? looks like they're all passing in CICD afaict

@jakeboone02
Copy link
Contributor

@jakeboone02 sounds reasonable! I attempted to add it here https://github.com/oven-sh/bun/blob/main/src/bunfig.zig#L592 and it didn't do anything - happen to know why off the top of your head before I debug?

I don't know but be aware of #12216 (although that seems like the opposite problem...🤷‍♂️).

@RiskyMH
Copy link
Member

RiskyMH commented Dec 19, 2024

which tests? looks like they're all passing in CICD afaict

I'm not sure why its green status, but if you press "details" it will send you to the page: https://buildkite.com/bun/bun/builds/8169

@martinamps
Copy link
Contributor Author

martinamps commented Dec 19, 2024

aha ty @RiskyMH should've clicked through. Fixed cc @Jarred-Sumner (2 fail on canary because my code isn't there yet I guess)

@jakeboone02 I agree with you it'd be nice to have it in bunfig but unclear to me immediately why that isn't working like --silent.. Ok if we make that a follow-up PR when I have more cycles?

@jakeboone02
Copy link
Contributor

@jakeboone02 I agree with you it'd be nice to have it in bunfig but unclear to me immediately why that isn't working like --silent.. Ok if we make that a follow-up PR when I have more cycles?

Sure! The sooner the better for the CLI flag IMO. A bunfig option can wait.

@Jarred-Sumner
Copy link
Collaborator

I woud like to merge this but there are Windows test failures

@martinamps
Copy link
Contributor Author

martinamps commented Dec 21, 2024

ah-- I think it's because windowsIsTerminal() does not respect FORCE_COLOR which is how I trigger the code path in the other environments https://github.com/oven-sh/bun/blob/main/src/cli/filter_run.zig#L534 -- now if you try use it in a non TTY environment it'll error, and I explicitly test that on windows. Hopefully passes this time 😅 sorry!

@Veskel01
Copy link

My goodness - I'm waiting for the merge. This is the most annoying thing about Bun-based monorepo

@Jarred-Sumner Jarred-Sumner merged commit a6ad3b9 into oven-sh:main Dec 22, 2024
63 checks passed
@Jarred-Sumner
Copy link
Collaborator

Thank you. This will be part of Bun v1.1.43

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.

5 participants