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

Nest test results under describe scopes #12189

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

jakeboone02
Copy link
Contributor

What does this PR do?

Updates the output of bun test so that describe scope names are only printed once and test/it results are nested and indented beneath their respective scopes.

How did you verify your code works?

There were no existing automated tests for the output format, so manual inspection has been the only test so far. I can write some tests if necessary.

  • I included a test for the new code, or existing tests cover it
  • I ran my tests locally and they pass (bun-debug test test-file-name.test)

Comparison of results

import { describe, expect, it } from "bun:test";

describe("our first test group", () => {
  describe("sub group 1", () => {
    it("knows zero is zero", () => { expect(0).toBe(0); });
    it("1 is the loneliest number", () => { expect(1).toBe(1); });
  });
  describe("nested group 2", () => {
    it("likes the number 2", () => { expect(2).toBe(2); });
    it.todo("thing 3 is still to do", () => { expect(3).toBe(0); });
  });
  it("here's thing 4", () => { expect(4).toBe(4); });
});

describe("some more tests", () => {
  it("does thing cinco", () => { expect(5).toBe(5); });
  it.skip("skips thing 6", () => { expect(6).toBe(6); });
  it("does thing {# days of the week}", () => { expect(7).toBe(7); });
});

image

@mangs
Copy link
Contributor

mangs commented Jun 27, 2024

@jakeboone02 This is so nice! Great job! One thing that I noticed while looking this over was the "new" example shows as being over 6x slower than before. Is that repeatable/consistent?

@jakeboone02
Copy link
Contributor Author

@jakeboone02 This is so nice! Great job! One thing that I noticed while looking this over was the "new" example shows as being over 6x slower than before. Is that repeatable/consistent?

Thanks! I think the speed difference is just because the new one is a debug build. I'll try a release build later tonight.

Copy link
Contributor

@jakeboone02, your commit has failing tests :(

💪 1 failing tests Darwin AARCH64

  • test/js/web/streams/streams.test.js 1 failing

🪟💻 1 failing tests Windows x64 baseline

  • test/integration/next-pages/test/dev-server.test.ts 1 failing

View logs

@jakeboone02
Copy link
Contributor Author

jakeboone02 commented Jun 27, 2024

@mangs I tried the release build from this PR with bunx bun-pr 12189 and the performance was basically equivalent (a tiny bit faster for some reason??):

$ hyperfine --warmup 50 'bun test ./bun-debug-test.test.ts' 'bun-12189 test ./bun-debug-test.test.ts'
Benchmark 1: bun test ./bun-debug-test.test.ts
  Time (mean ± σ):      12.5 ms ±   2.2 ms    [User: 8.3 ms, System: 8.3 ms]
  Range (min … max):     9.9 ms …  36.4 ms    234 runs

  Warning: Statistical outliers were detected. Consider re-running this benchmark on a quiet PC without any interferences from other programs. It might help to use the '--warmup' or '--prepare' options.

Benchmark 2: bun-12189 test ./bun-debug-test.test.ts
  Time (mean ± σ):      11.8 ms ±   1.6 ms    [User: 7.5 ms, System: 8.5 ms]
  Range (min … max):     9.5 ms …  23.4 ms    209 runs

  Warning: Statistical outliers were detected. Consider re-running this benchmark on a quiet PC without any interferences from other programs. It might help to use the '--warmup' or '--prepare' options.

Summary
  'bun-12189 test ./bun-debug-test.test.ts' ran
    1.06 ± 0.23 times faster than 'bun test ./bun-debug-test.test.ts'

@Jarred-Sumner I have a feeling this speed difference is insignificant and/or inconsistent, but I'd love to know if I accidentally made it faster somehow.

EDIT: after some more benchmarking, it does appear to have made very little perf impact, if any.

Copy link
Contributor Author

@jakeboone02 jakeboone02 left a comment

Choose a reason for hiding this comment

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

I added commit c2c667a that better aligns the message for when a .todo test passes and the --todo flag has been set. I didn't really know what I was doing so I copied similar code from here:

fn printTestLine(label: string, elapsed_ns: u64, parent: ?*jest.DescribeScope, comptime skip: bool, writer: anytype) void {
var scopes_stack = std.BoundedArray(*jest.DescribeScope, 64).init(0) catch unreachable;
var parent_ = parent;
while (parent_) |scope| {
scopes_stack.append(scope) catch break;
parent_ = scope.parent;
}
const scopes: []*jest.DescribeScope = scopes_stack.slice();

That seems like a lot of code just to get the number of items in a linked list, so please let me know if there's a more efficient way.

Copy link
Collaborator

@dylan-conway dylan-conway left a comment

Choose a reason for hiding this comment

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

Nice, I like this.

Doesn't need to happen in this pr, but I think printing a status indicator for describe scopes would also be an improvement. Vitest does this, it makes the scopes look a little less out of place

Screenshot 2024-06-28 at 2 01 32 PM

jest for comparison:

Screenshot 2024-06-28 at 2 11 16 PM

@jakeboone02
Copy link
Contributor Author

Yeah, I had the same thought. Needs statusing at the file level, too. I think bun test kind of prints the test results as it goes through them, so it might require some refactoring to get the overall status of each file/describe and then print everything.

Other than that, is this ready after I commit that suggestion about the BoundedArray?

@dylan-conway
Copy link
Collaborator

Yeah, I had the same thought. Needs statusing at the file level, too. I think bun test kind of prints the test results as it goes through them, so it might require some refactoring to get the overall status of each file/describe and then print everything.

Yeah, we will probably need to store the results and print either after a describe scope finishes or maybe after the file finishes.

Other than that, is this ready after I commit that suggestion about the BoundedArray?

Yep I'd say it's good to go after that

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.

Nest test/it calls under a single describe label instead of repeating the describe label for each test
3 participants