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

Summarize the max severity at the top of logs, star all max-severity logs #3163

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

Conversation

kainino0x
Copy link
Collaborator

@kainino0x kainino0x commented Nov 13, 2023

This is an alternative way of highlighting info to #3156. It avoids printing logs multiple times since we have problems with some tests printing too much. It also applies to all runtimes, not just standalone, so that's more important.

image

Issue: None


Requirements for PR author:

  • All missing test coverage is tracked with "TODO" or .unimplemented().
  • New helpers are /** documented */ and new helper files are found in helper_index.txt.
  • Test behaves as expected in a WebGPU implementation. (If not passing, explain above.)

Requirements for reviewer sign-off:

  • Tests are properly located in the test tree.
  • Test descriptions allow a reader to "read only the test plans and evaluate coverage completeness", and accurately reflect the test code.
  • Tests provide complete coverage (including validation control cases). Missing coverage MUST be covered by TODOs.
  • Helpers and types promote readability and maintainability.

When landing this PR, be sure to make any necessary issue status updates.

@greggman
Copy link
Contributor

So that's better than it was because it at least it says there was an exception but at a glance it looks like I still need to scroll down 30 screens to see the real error, right? (sorry, out of the office and on slow internet so not easy to check)

@greggman
Copy link
Contributor

I would like to ask about the sorting. Maybe this would have helped but, here for example is a report before this change

Screenshot 2023-11-27 at 14 15 39

vs after this change

Screenshot 2023-11-27 at 14 15 08

vs the change suggested in #3156

Screenshot 2023-11-27 at 14 17 10

In the first one I've mislead myself multiple times into thinking there was an implementation error when in fact it was a CTS error.

The 2nd one helps in that I can see in small print that there was an exception but I still have to scroll down, possibly many pages, to see it.

In the 3rd the issue is right at the top.

Can we put in both? Is there something wrong with the sorting? Should I add official categories to the sort one to make it more palatable or does sorting have issues?

@kainino0x
Copy link
Collaborator Author

We can do both. I wanted to have a solution that would apply to more than just standalone since a lot of people don't use it. But since yours does only affect standalone it won't cause any extra log file spew so I think it would be OK.

That said I think this change is going to break l.name === 'EXCEPTION' so we should probably have a more robust way of doing that (expando property holding the severity, maybe?)

@kainino0x
Copy link
Collaborator Author

That said I think this change is going to break l.name === 'EXCEPTION'

Wait, never mind, that was a previous revision of this PR. I made it not do that.

@kainino0x kainino0x enabled auto-merge (squash) December 1, 2023 03:02
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.

2 participants