1655952 - Consider configurably moving the test info box to the super sidebar / super navigation panel

Status: NEW

(Webtools :: Searchfox, defect)

Description

[Olli Pettay [:smaug][[e-mail]]]

Could the warnings and such be moved outside the basic UI, perhaps to the Navigation thingie on the right.

Comment 1

[Andrew Sutherland [:asuth] (he/him)]

To expand on my design rationale from bug 1653986:

• Motivations:
  • There's a serious problem in mozilla-central with intermittent and disabled tests.
  • There's a less serious problem of tests that take a long time to run, but in practice it's actually quite hard to be aware of how long a test takes to run. Even if you're running the single test yourself from the command-line, the extensive test runner overhead can hide the issue and encourages people to context-switch to something else.
  • There's a real risk of someone using searchfox to check for existing tests that ostensibly check some invariant, see test logic that should be doing the check, but not realize the code is disabled.
  • The existing mechanisms we have for dealing with intermittent and disabled tests are bugzilla-centric.
    • Treeherder does possess an Intermittent Failures View but it's also rather bugzilla-centric, with its sorting/faceting filters being: bug, count, bugzilla product, bugzilla component, summary (which does include the test path usually), whiteboard.
    • I posit this works well if someone wants to explicitly context-switch into explicitly fixing intermittent and disabled tests, but is not an ambient awareness mechanism, and people are frequently so busy that there is never an opportunity to explicitly switch into a test-fixing mode.
    • There is a mechanism by which the "Intermittent Failures Robot" notifies about all intermittent failures for the preceding week on Mondays, but this results in an onslaught of bugmail for bugs with as little as one intermittent up through all intermittents.
• Understanding of Searchfox's UI as perceived by users:
  • Anecdotal: I've ended up pointing out the navigation panel on the right to people multiple times for its myriad purposes. Specifically for: awareness of what component a file belongs to, the permalink mechanism, the log mechanism, the hg web mechanism, the code coverage mechanism. (I don't think anyone even knows about DXR anymore.)
  • Theory: I suspect some combination of people first using searchfox viewing it as an esoteric menu of things that are distracting them and so they ignore, plus the web generally being filled with ads and people ending up emergently trained to ignore/visually pre-filter-out things that look like banner ads.
  • The navigation panel on the right says "Navigation" and the only contextual information provided is the bugzilla component which may not actually seem like information until people think about it.
  • The navigation panel overlays content.
    • In my Firefox usage using 2 4k monitors in landscape with 2 Firefox windows tiled horizontally per monitor using Tree Style Tabs, I can see 132 columns of text of this file before the navigation panel occludes the content. I worry that include more prose-focused text in the navigation box would require it to become wider, potentially encroaching on the 120 column (less common) and 100 column (more common) boundaries.
• Design Rationale Proper:
  • I posit we do want people to be confronted by the information that the information that a test results in (possibly intermittent errors), is skipped/disabled, or runs very slowly.
    • The information is useful for those who might otherwise assume the test is checking an invariant that does not hold at all or only intermittently holds.
    • The information is useful for creating serendipitous test fixes where someone is looking at a test, sees that it has problems that should perhaps be fixed, and is able to quickly come up with a fix.
  • I posit that putting the information in the sidebar means people are much less likely to see the information, even if they want to see it.
  • The scenario where the test info box is green and is just an FYI has less justification for putting itself in the content stream, as there usually won't be anything to address. That said:
    • That's the motivation for making the box green, to support visual pre-filtering of the box as something that can be ignored.
    • The length of time the test takes to run is useful information and ensuring tests run quickly is important to the health of the test ecosystem because:
      • Test time costs money.
      • Slow tests frequently run the risk of being intermittent timeout failures, especially because the slowness isn't always intentional to the test.
      • Slow tests are frustrating to developers trying to run tests locally.
    • The presence of the green box helps provide a confirmation that Searchfox properly understands the current file to be a test and that the system is actually running the test. Files that aren't in a test manifest won't be run and won't have green boxes. If there was no (green) box displayed in the information (non-warning, non-error) case, this would create ambiguity about whether the test was actually understood to be a test that runs, etc.
  • Test files usually contain a bunch of boilerplate at the top, so something additional to scroll over didn't seem like a major hurdle.
    • My ideal tests have large block comments that explain what they're doing, and because searchfox syntax highlights comments in their own color, this allows for visual pre-filtering to help me quickly locate the comment on the page as long as it's "above the fold" (visible without scrolling down).

That context provided, can you elaborate on "clutter the main UI" and how it impacts your usage of searchfox (for good and bad, and in relation to the goals/tradeoffs of the info boxes described above)? In particular, I'm interested in knowing:

• The document.body.offsetWidth (or more standards compliant choice) of your searchfox window. Mine is 1304, but if yours is much larger, leaving you with a large amount of unused whitespace, perhaps it would be appropriate to use a CSS media query to place the info box in a second column on the page that's separate from the navigation box or subsumes it?
• Is there something searchfox could be doing in terms of semantic highlighting of the test body in order to make it easier to visually pre-filter to or jump to (via the navigation panel or other) the points of interest in the test file? For example, providing a (collapsible/hide-able) table-of-contents list of the top-level functions and class definitions in the test and hot-keys like 'j' and 'k' to quickly navigate navigate between these top-level definitions (like one might do with meta-{ and meta-} in emacs, etc.) that work even without the TOC visible?

Bonus points for using devtools to alter the DOM/CSS of the page to create a mockup and take a screenshot that you attach to the bug!

Comment 2

[Andrew Sutherland [:asuth] (he/him)]

We had some further discussion in-channel at https://matrix.to/#/!vDKxYNxlsZYvjSWGBh:mozilla.org/$_tL5jDFANqM54zXNIXpgBiMBzcaQH-X7WlqWrYHaqSI?via=mozilla.org&via=matrix.org&via=t2bot.io with :smaug using userContent.css to hide the info box, so I'm clearing the needinfo, but I'd still find it invaluable if you have time to respond in the future.

I'm going to leave the bug open for discussions on how to enhance this functionality and its trade-offs.

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

No one has still explained to me why the errors are now suddenly the most important part of mostly code indexing tool when looking at the test files.

document.body.offsetWidth is 1920

Comment 4

[Kartikaya Gupta (email:[e-mail])]

There's a bunch of false assumptions built into your question, but the face value answer is "if you're doing something with a test and don't care if it's even enabled, then you're doing it wrong". Even if you're just changing some API usage on a disabled test, you should care that the test is disabled because it means that API usage isn't real and your code isn't being exercised the way you think it is.

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

Attachment: Screenshot_2020-07-29 test_presentation_dc_sender html - mozsearch.png

Something simple like this would keep the main UI simple and clean.

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #4)

There's a bunch of false assumptions built into your question, but the face value answer is "if you're doing something with a test and don't care if it's even enabled, then you're doing it wrong". Even if you're just changing some API usage on a disabled test, you should care that the test is disabled because it means that API usage isn't real and your code isn't being exercised the way you think it is.

Well, the warning doesn't seem to be about disabled tests only, but also about test failures. Why do I need to care about test failures first before looking what the file is about?

Comment 7

[Kartikaya Gupta (email:[e-mail])]

(In reply to Olli Pettay [:smaug] from comment #6)

Well, the warning doesn't seem to be about disabled tests only, but also about test failures. Why do I need to care about test failures first before looking what the file is about?

Yeah, arguably you don't for that specific use case. This is where I fall back to "initial implementation, not perfect, suggestions for refinement are welcome". Thanks for the screenshot you posted in comment 5, that does look pretty reasonable although I share :asuth's concern about the navigation panel ending up too wide or covering up too much of the content.

I think it might be reasonable to explore a UI change where instead of having the navigation thing float over top of the code, we dock it inside a pane on the right side of the page, and that pane can contain other contextual information such as the test statistics. If the code is wider than the remaining space (browser width minus pane width) then it would scroll horizontally.