Skip to content

refactor: improve e2e test reporting #10304

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

Merged
merged 9 commits into from
Oct 17, 2023
Merged

refactor: improve e2e test reporting #10304

merged 9 commits into from
Oct 17, 2023

Conversation

aslilac
Copy link
Member

@aslilac aslilac commented Oct 16, 2023

Screenshot 2023-10-16 at 4 14 34 PM
  • Use some buffering to store test output until the test is complete, and then only output it if the test failed, finally releasing the memory. Makes finding failures a lot easier, as even successful tests end up having a number of occurrences of the word "error" in their output. Trimming these from the logs makes finding the actual errors much easier.

  • Also counts how many tests succeed, and tracks specific test failures to show a little summarized report at the end of the output.

  • Better handling of printing errors, to make them easier to interpret and locate (especially when a source location is given)

  • An entire successfully run can now fit in a single screenshot! If a test fails, any output you see is much more likely to now actually be related.

@aslilac aslilac changed the title refactor: better e2e test reporting refactor: improve e2e test reporting Oct 16, 2023
@aslilac aslilac requested review from a team, Parkreiner and mtojek and removed request for a team and Parkreiner October 16, 2023 18:54
"",
)}`,
);
onStdErr(chunk: string, test?: TestCase, _?: TestResult): void {
Copy link
Member

Choose a reason for hiding this comment

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

We might be losing here output from coderd, which can be really useful during backend debugging. In the legacy version, it is logged like this:

[stderr] [unknown]: [WebServer] 2023-10-17 06:54:03.412 [info]  provisionerd.runner: apply successful  job_id=89020454-1bd8-4eed-a0c9-eb12fa532a92  template_name=68ac3052  template_version=hardcore_moser9  workspace_build_id=465c8cad-3fcb-42fb-b9bb-56007b4a1db3  workspace_id=74006ba8-82a8-4693-b971-f3da61ad02e0  workspace_name=d93260bd  workspace_owner=admin  workspace_transition=start  resource_count=1  resources="[name:\"example\" type:\"echo\"]"  state_len=0

It looks like the new reporter simply skips these log records.

Copy link
Member Author

Choose a reason for hiding this comment

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

that was sort of intentional. it adds a lot of noise when trying to debug a frontend failure, and since it's not tied to a specific test case it's hard to buffer and display later in a way that makes sense.

I also added some logic for preserveOutput, but I just read the docs and it turns out it does something entirely different from what I expected, and can't even be set from the command line like I hoped 😵‍💫

Copy link
Member Author

Choose a reason for hiding this comment

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

the idea was supposed to be that backend peeps could set --preserveOutput=always when they ran the tests... I'd really like to find a compromise here to keep the output clean when possible. it's just always such a pain to find the actual error in all of the mountains of logs.

Copy link
Member Author

Choose a reason for hiding this comment

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

example run, which now has 41 occurrences of the word "error" but succeeded https://github.com/coder/coder/actions/runs/6549912328/job/17787879014?pr=10304

Copy link
Member

Choose a reason for hiding this comment

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

A good compromise would be only filtering out audit_log entries, which do not bring a lot of value. 👍

I would leave coderd output as it is helpful with debugging the request flow or provisioner behavior, especially while dealing with flaky issues.

Copy link
Member Author

Choose a reason for hiding this comment

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

if we can also ignore lines like echo: recv done on Session session_id= error=EOF then I'll be happy. :)

Copy link
Member

Choose a reason for hiding this comment

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

Agree 👍

@Parkreiner
Copy link
Member

Parkreiner commented Oct 17, 2023

Oh, just realized that I had gotten removed from the reviewer list after Marcin jumped in. Sorry about that

@aslilac
Copy link
Member Author

aslilac commented Oct 17, 2023

Oh, just realized that I had gotten removed from the reviewer list after Marcin jumped in. Sorry about that

yeah, I tried assigning coder/ts, and then realized "oh, he's gonna have like zero context on this change" 😂 went and looked at the blame to see who actually would

Copy link
Member

@mtojek mtojek left a comment

Choose a reason for hiding this comment

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

LGTM

@aslilac aslilac merged commit 2b5e02f into main Oct 17, 2023
@aslilac aslilac deleted the better-playwright-reporter branch October 17, 2023 22:11
@github-actions github-actions bot locked and limited conversation to collaborators Oct 17, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants