-
Notifications
You must be signed in to change notification settings - Fork 475
Move apps filtering to client side #3094
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
base: main
Are you sure you want to change the base?
Conversation
|
One task: I deleted the test that was verifying the behavior for that feature. I'd like to know what approach we should follow here, since a simple liveview test isn't possible for this feature anymore. Should we use a headless browser test? Asking because we don't have any other type of that test yet. |
| assert render(view) =~ slug | ||
| end | ||
|
|
||
| test "filter the apps based on slug, name and app folder", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since there's no LiveView test for that, what about adding a JS test? So we can ensure it will filter the apps based on the search criteria
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it is worth adding the complexity of browser testing for this feature. One option is to use hooks (or another client-side abstraction), and unit test that instead, like we do here: https://github.com/livebook-dev/livebook/tree/main/assets/test/hooks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The hooks directory actually only has unit tests for some specific encapsulated classes that we use from hooks. I don't think there's an easy way to unit test it, because it requires the page structure.
But I don't think adding browser testing for this specifically is worth it either.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think there's an easy way to unit test it, because it requires the page structure.
Exactly.
But I don't think adding browser testing for this specifically is worth it either.
The /apps page is the home page for Livebook apps' users. It can be the first page internal users of Livebook app sees whenever they're using their Livebook-based internal tools.
Given that, I’d say this page is important enough to justify having end-to-end tests for it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then move the rendering of the apps to the client, using a simple rendering like lit? Or postpone this pull request. I am strongly against adding end-to-end tests, it will add more complexity to a test suite that is already complex because of how teams are setup, and it will impact all Livebook contributors. If this was on the teams side, I would have fewer complaints.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then move the rendering of the apps to the client, using a simple rendering like lit?
I think that would add even more unnecessary complexity than having an end-to-end test.
I am strongly against adding end-to-end tests, it will add more complexity to a test suite that is already complex because of how teams are setup
I understand the cost.
One thing we can do is run those tests by default only in the CI, as we already do for other parts of the test suite, so contributors don't have to deal with them.
| selectedFolder === "" || appFolderId === selectedFolder; | ||
|
|
||
| if (matchesSearch && matchesFolder) { | ||
| card.style.display = ""; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should use something like this.js().show(card) and this.js().hide(card) (docs), otherwise LV rerender can override the page with unfiltered state.
| group.style.display = ""; | ||
| const countElement = group.querySelector("[data-group-count]"); | ||
| if (countElement) { | ||
| countElement.textContent = `(${count})`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similarly here, we need phx-update="ignore" on this element. And I think we also need ignore on the parent of input/select.
To test you can deploy a new app in another tab and see which state gets incorrectly reverted.
| assert render(view) =~ slug | ||
| end | ||
|
|
||
| test "filter the apps based on slug, name and app folder", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The hooks directory actually only has unit tests for some specific encapsulated classes that we use from hooks. I don't think there's an easy way to unit test it, because it requires the page structure.
But I don't think adding browser testing for this specifically is worth it either.
Since filtering apps don't alter any server state, I think we can move this to the client side to improve the user experience.
Before
Simulating a 300ms latency with
window.liveSocket.enableLatencySim(300)CleanShot.2025-11-14.at.16.08.32.mp4
After
Moving to the client
CleanShot.2025-11-14.at.16.09.42.mp4
Out of curiosity, this was mostly done by Tidewave. 😎