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

Add doctest circle indicator on editor (Looking for early feedback) #1789

Closed
wants to merge 0 commits into from

Conversation

Sleepful
Copy link
Contributor

After running doctests inside livebook, add a circle inside the editor to indicate the iex> prompts that succeeded or failed after evaluation.

Example:
Screen Shot 2023-03-15 at 02 20 01

Currently lacking some details but I tried to illustrate the main idea before turning it into a functional implementation. I want to get some approval on what I intend to do before moving into coding it properly. Do tell me if any of the commented code looks like it is in the wrong place. I also left a question in one of the comments.

tests =
test_module.tests
|> Enum.sort_by(& &1.tags.doctest_line)
|> Enum.map(&run_test/1)

formatted = format_results(tests)
put_output({:text, formatted})
# put_output({:doctest_results, doctest_results})
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this would be the line which initiates the signal that should arrive at the editor to indicate doctest line numbers

Comment on lines 1717 to 1719
# I am under hte assumption that in some place inside
# this method the operations get sent to session_live.ex,
# is this correct? If so, which code is in charge of doing so?
Copy link
Contributor Author

Choose a reason for hiding this comment

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

a little lost here. It looks like session_live does get the :add_cell_evaluation_output operation from this function. Where does that happen?

Comment on lines 1972 to 1978
# {:add_cell_doctest, }
# similar to :add_cell_evaluation_output but instead
# this will add the information of doctests that
# pass or fail and their line number
# to the CellEditor attributes, then the CellEditor
# will react to it through a MutationObserver to
# apply the styling into the Monaco editor
Copy link
Contributor Author

@Sleepful Sleepful Mar 15, 2023

Choose a reason for hiding this comment

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

this would send an update to the cell_editor_component in order to update its data-doctest-results attribute, that's how the editor will know to update doctest decorations

@Sleepful Sleepful changed the title Add doctest circle indicator on editor (ROUGHT DRAFT) Add doctest circle indicator on editor (ROUGH DRAFT) Mar 15, 2023
Comment on lines 248 to 253
# defp io_request({:livebook_put_doctest, output}, state) do
# state = flush_buffer(state)
# send(state.send_to, {:runtime_doctest_output, state.ref, output})
# {:ok, state}
# end

Copy link
Contributor Author

Choose a reason for hiding this comment

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

not sure why/what is the function of io_proxy.ex but I am following the same pattern for delivering messages as :livebook_put_output, which goes through this module

@Sleepful Sleepful changed the title Add doctest circle indicator on editor (ROUGH DRAFT) Add doctest circle indicator on editor (Looking for early feedback) Mar 19, 2023
@jonatanklosko
Copy link
Member

jonatanklosko commented Mar 22, 2023

Hey @Sleepful, thanks for the PR and sorry for the late feedback!

I would start with this flow:

  1. When doctests finish send a special output {:doctests_result, result}, where result includes information about each doctest line number and status. Let's keep the regular output we send currently as is for now.

  2. We render the result as live component, which does push_event("doctests_result:#{cell_id}", ...) and we use it to update the editor on the client, similarly to

    this.handleEvent(
    `evaluation_finished:${this.props.cellId}`,
    ({ code_error }) => {
    liveEditor.setCodeErrorMarker(code_error);
    }
    );
    For now the component can render empty body, but I think we will use it to render grouped tests results (Group doctests output #1526).

  3. Theoretically there can be multiple modules with doctests in a cell, so we need to clear the decorations only on new evaluation. To do that we can add a clause in SessionLive for handle_action(socket, {:start_evaluation, cell, section}) and do a push_event to that cell to clear the decorations.

@josevalim if you have any concerns with this flow let me know :)

Also, note to self: it's fine to change Runtime messaging around doctests at any point, because both the sending and receiving side is in Livebook (and not in Kino).

@Sleepful
Copy link
Contributor Author

Sleepful commented Mar 31, 2023

@jonatanklosko no worries on delay. Your explanation is crystal clear, thanks for that! I will delay a little bit on the delivery, I hope that's okay, so just FYI I will have an update with code in approximately 2 weeks. TTYL 😁

@jonatanklosko
Copy link
Member

@Sleepful sure, there's definitely no rush!

@Sleepful
Copy link
Contributor Author

I did not mean to close it, oops 😅
I will open a new one though!

@aleDsz aleDsz temporarily deployed to uffizzi May 12, 2023 21:24 — with GitHub Actions Inactive
@aleDsz aleDsz temporarily deployed to uffizzi May 15, 2023 15:13 — with GitHub Actions Inactive
@aleDsz aleDsz temporarily deployed to uffizzi May 15, 2023 17:27 — with GitHub Actions Inactive
@Sleepful
Copy link
Contributor Author

@jonatanklosko one question, does push_event arrive at the client in the right order every time?

My concern might be unfounded, but suppose push_event("start_evaluation:#{cell_id}" arrives after push_event("doctest_result:#{cell_id}" for some odd reason, it would be a bug.

Typical HTTP request issue, but we are using LiveView so probably a non-issue.

@jonatanklosko
Copy link
Member

@Sleepful the events arrive at the same order as they are sent from the server. On a technical side, events go via the same websocket connection :)

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.

3 participants