The tooltips for the left panel of stream settings
have been broken since November 2018 due to my
commit 8f915da2ca.
The code prior to 2018 was restoring tooltips
right inside the loop where we were detaching
the row from the DOM to put it back into the
DOM at another place. And then I tried to
just add them in bulk, forgetting that I was
in the middle of all the DOM manipulation (and
hence my selector for the loop was a noop).
Also, I don't think we've ever had them for live
events that add streams. (I fixed that too.)
It's not clear to me that this code is actually
necessary, as we get hover help without
calling $(...).tooltip(...) properly.
This is probably why we didn't notice any
breakage when we merged my 2018 commit.
We generally want to avoid clicking on DOM elements
that may not actually be visible due to the prior
operation. Instead, we can just find the visible
element after each step.
I also introduce a couple helper functions to
de-clutter the click/unclick/click steps, and I do
a couple extra clicks for good measure.
You can verify that the test will fail if you
add an early return to update_check_button_for_sub.
We just want to reset the scrollbar here, which
we still do via ui.reset_scrollbar.
You don't want to preserve scroll position if
you are filtering or re-sorting.
We have long had this annoying two-pass way of building the
DOM that I am trying to eliminate.
The function names that I introduce here describe the current
situation more accurately.
In passing I make it so that we only throttle redraws when
users are actually typing. Using a throttled redraw when
you click on the sort icons is at best unnecessary, and it
may actually aggravate double clicks.
This splits the one big `run_test` block into
add/edit/delete parts, for better clarity.
This also removes the `with_field` calls and
uses the override style instead, which is
preferred because it makes sure the stubbed
function is actually called.
This commit replaces `with_field` calls to
use the override style instead.
`override` is preferred since it makes sure
the stubbed function is actually called,
while `with_field` doesn't, which makes it
hard to spot dead code.
This commit replaces all `with_stub` calls and
explicitly calls `make_stub` instead.
The `with_stub` helper does not add much clarity
hence we now use scoped stub objects instead.
This de-indents some blocks where scoping isn't
required, for example when there is a single
stub object inside a `run_test` function.
With this change, we also need to explicitly
assert `num_calls`.
We can relax this restriction in the future, but
basically every time it came up for me, the test
code was just disorganized, or it had an easy
workaround.
These are still kind of a mess.
The old code combined the worst of both worlds:
- we had one monolithic test
- we called the events multiple times,
verifying a different stub each time
Now I make the tests more granular.
We could actually re-combine the tests, but
in a nicer way, so that we just set
up multiple stubs and verify that all stubs
get correctly invoked.
There is also some code cleanup here--in dispatch_subs,
we don't stub stream_data, so it's easier to write
deeper tests that actually validate the data changes.
This prevents a bug where we interpret "2something"
as a modern slug instead of a legacy stream name.
The bug was probably somewhat unlikely to happen in
practice, since it only manifests if 2 is an actual
stream_id.
Steve Howell
Abhijeet Prasad Bodas
Anders Kaseorg