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

feat(cli/test): improve test output readability #24357

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

hukuuu
Copy link

@hukuuu hukuuu commented Jun 27, 2024

Indent last line of tests that contain multiple steps.

Closes #24071

@CLAassistant
Copy link

CLAassistant commented Jun 27, 2024

CLA assistant check
All committers have signed the CLA.

@hukuuu
Copy link
Author

hukuuu commented Jun 27, 2024

@iuioiua after a but of struggle i think i managed to fix this. Couple of things to note:

  1. The implementation breaks when tests have "parallel steps" for example, in cargo test test::steps_invalid_usage:
Deno.test("parallel steps with sanitizers", async (t) => {
  // not allowed because steps with sanitizers cannot be run in parallel
  const step1Entered = Promise.withResolvers<void>();
  const testFinished = Promise.withResolvers<void>();
  t.step("step 1", async () => {
    step1Entered.resolve();
    await testFinished.promise;
  });
  await step1Entered.promise;
  await t.step("step 2", () => {});
});

Produces

parallel steps with sanitizers ...
  step 1 ... INCOMPLETE
    step 2 ... FAILED (2ms)
  parallel steps with sanitizers ... FAILED (due to incomplete steps) (3ms)

I think its because of the TestEvents not firing properly ( because of how the deno test is written, step 1 waits for step 2, which should be invalid thing. I made the cargo test pass by touching the .out file

  1. We now indent tests that have only output ( and no nested steps ):
sync pass todo ...
------- output -------
Warning: Not implemented: test.TestContext.todo
----- output end -----
  sync pass todo ... ok (3ms)

I am not sure if this looks better or worse.

What are your thoughts?

@iuioiua iuioiua changed the title feat(cli/test): improve test output readability (#24071) feat(cli/test): improve test output readability Jun 28, 2024
Copy link
Collaborator

@iuioiua iuioiua left a comment

Choose a reason for hiding this comment

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

This doesn't seem like the correct approach. My understanding was that we just needed to prepend a couple of spaces, or increase description.level by 1, in the following lines:



Can you try that?

Also, looking at the output, I'm having 2nd thoughts on whether we should do the indent. Perhaps, just having the grayed text will suffice. Either way, let's see how it all looks 🙂

Indent last line of tests that contain multiple steps.
@hukuuu
Copy link
Author

hukuuu commented Jun 29, 2024

Following your suggestion verbatim doesnt produce the desired result. Couple of screenshots ( i have put an x in front just to make things easier to see )

image image

Here is the test output of an example project i use to test this:

image

There are two problems here:

  1. My understanding is that tests and steps should be indented if they have children. With your suggestion only steps are being indented.
  2. All steps are being indented, instead of only those who have children

--

I think we must have a condition to understand wether a test/step is actually a parent. My best try to infer that is to see whether we are in a new line while printing the result. If the test/step doesnt have children, its result will be printed at the same line as the description ( self.is_new_line is false ). Conversely, if self.is_new_line is true, that means that the test/step in question is resuming the printing after its children have finished printing.

Maybe, another way to infer the same would be to peek at the child_results_buffer lenght.

EDIT: actually, checking if there is an entry in the child_results_buffer for the current test/step works as well, just tried it:

image

Also, even just to make things grey, i think the same inference should happen, otherwise pretty much all test output will become grey

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.

Make test output slightly easier to read
3 participants