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

Use papyros to debug testcases #5336

Merged
merged 20 commits into from
Feb 13, 2024
Merged

Use papyros to debug testcases #5336

merged 20 commits into from
Feb 13, 2024

Conversation

jorg-vr
Copy link
Contributor

@jorg-vr jorg-vr commented Feb 2, 2024

This pull request introduces the new papyros debugger as replacement for the old tutor based debugger.

This contains a new version of papyros (dodona-edu/papyros#615), which adds:

  • support for test code lines added at the end of the file
  • support for files provided by a js function

This allows us to fully setup a testcase by prefilling the input, adding extra testcode lines and providing all files used by the testcase.

image

The testcode is clearly separated from the student code using a background color and an explanatory comment.
image

There are two icons that allow the student to remove this code or edit it. When the edit option is chosen it just becomes part of the student code:
image

To be able to use the debugger, papyros is now also available on the submission detail and feedback show pages.

Adding this allows removing our dependency on the old python tutor frontend code, the pyodide-trace-library and the fscreen dependency.

Related to: #5014

Note: if approved, I will release a related non-beta papyros version to use

@jorg-vr jorg-vr added the feature New feature or request label Feb 2, 2024
@jorg-vr jorg-vr self-assigned this Feb 2, 2024
@jorg-vr jorg-vr added the deploy naos Request a deployment on naos label Feb 6, 2024
@github-actions github-actions bot removed the deploy naos Request a deployment on naos label Feb 6, 2024
@jorg-vr jorg-vr added the deploy naos Request a deployment on naos label Feb 6, 2024
@github-actions github-actions bot removed the deploy naos Request a deployment on naos label Feb 6, 2024
@jorg-vr jorg-vr added the deploy naos Request a deployment on naos label Feb 6, 2024
@github-actions github-actions bot removed the deploy naos Request a deployment on naos label Feb 6, 2024
@jorg-vr jorg-vr added the deploy naos Request a deployment on naos label Feb 9, 2024
@github-actions github-actions bot removed the deploy naos Request a deployment on naos label Feb 9, 2024
@jorg-vr jorg-vr added the deploy naos Request a deployment on naos label Feb 9, 2024
@github-actions github-actions bot removed the deploy naos Request a deployment on naos label Feb 9, 2024
@jorg-vr jorg-vr requested review from bmesuere and chvp and removed request for a team February 9, 2024 16:00
@bmesuere bmesuere added the deploy naos Request a deployment on naos label Feb 11, 2024
@github-actions github-actions bot removed the deploy naos Request a deployment on naos label Feb 11, 2024
@bmesuere bmesuere added the deploy mestra Request a deployment on mestra label Feb 11, 2024
@github-actions github-actions bot removed the deploy mestra Request a deployment on mestra label Feb 11, 2024
Copy link
Member

@bmesuere bmesuere left a comment

Choose a reason for hiding this comment

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

This is amazing work, @jorg-vr! This gets rid of a lot of legacy code, improves the current experience, allows us to add an even better integration in the future and is incredibly fast!

I encountered a small issue when running a simple input/output exercise (sum of 2 numbers): the input of the test was correctly loaded into the input area, but the lines were marked as checked from the start.

I don't know if possible, but can you add a little more whitespace between the real code and the test code?

Some other suggestions, probably for a later PR:

  • If you click debug, wait for the traces to generate and then edit the code you are still debugging the old code. In addition, there is no good way to make the code arrow go away. I would enter a "debug mode" when clicking debug which restricts some features to make this more clear (show stop button, read only editor).
  • Remove the "run your code" button and add a new, more prominent button as discussed with the previous PR
  • When debugging a test from the feedback table, it would make sense to also show that test in the modal. This might help figuring out what the expected value/output was.
  • The "code arrow" doesn't scroll into view when it's not visible. Maybe we can reuse the active line feature of the editor?

@@ -12,7 +12,7 @@ const CODE_EDITOR_PARENT_ID = "scratchpad-editor-wrapper";
const PANEL_PARENT_ID = "scratchpad-panel-wrapper";
const CODE_OUTPUT_PARENT_ID = "scratchpad-output-wrapper";
const CODE_INPUT_PARENT_ID = "scratchpad-input-wrapper";
const OFFCANVAS_ID = "scratchpad-offcanvas";
export const OFFCANVAS_ID = "scratchpad-offcanvas";
Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason to add export here and not use the export at the bottom as we do in our other files?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have a personal preference for this style. It is sometimes useful to know whether something is exported when looking at the definition. I have never had a use case where I needed a quick overview of all exported definitions from a file.

I have almost always used this style over the past years, except when explicitly modifying older files.
This has caused for a divergence in styles across files. I am open to fixing this in another pr.

Copy link
Member

Choose a reason for hiding this comment

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

I liked the idea of having a clear overview in the "old" style, but have no strong opinion about this. I just noticed a mix of two styles. According to "The Internet" multiple exports from the same file is a code smell. We can discuss this in person over lunch or coffee ;)

app/assets/javascripts/coding_scratchpad.ts Outdated Show resolved Hide resolved
app/assets/javascripts/coding_scratchpad.ts Show resolved Hide resolved
app/assets/javascripts/coding_scratchpad.ts Show resolved Hide resolved
app/assets/javascripts/tutor.ts Outdated Show resolved Hide resolved
@chvp chvp added the deploy naos Request a deployment on naos label Feb 13, 2024
@jorg-vr jorg-vr merged commit bcb1b9f into main Feb 13, 2024
12 of 13 checks passed
@jorg-vr jorg-vr deleted the feat/debug-with-papyros branch February 13, 2024 10:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deploy naos Request a deployment on naos feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants