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

Fix evaluation error when no evaluation code is provided #1472

Merged
merged 4 commits into from
Aug 28, 2023

Conversation

sayomaki
Copy link
Contributor

Description

This PR aims to resolve a bug where if there is no code to be evaluated in the playground stepper, an error will be thrown in the page (see attached image below:)

Screenshot 2023-08-23 at 11-49-10 Source Academy

The bug can be replicated by simply executing empty code in the playground stepper, or when modules are imported but no code has been provided, e.g.

import { heart } from "rune";

The first commit attempts to resolve the problem, but no useful information is provided back to the user (looks as if nothing has occurred) (maybe this could be detected and implemented via the frontend?), and thus the second commit will return an evaluation response with a message to signal that nothing has been evaluated (see images below):

New behaviour when nothing is evaluated (click to expand)

Screenshot 2023-08-23 at 11-47-49 Source Academy

Screenshot 2023-08-23 at 11-47-37 Source Academy

If the second commit/user interaction feedback is not required (e.g. implemented in frontend), feel free to let me know and I will remove the commit from this PR.

Testing

These changes have been tested locally, and no longer produces the error as shown above in the first image.

* Resolves bug where no execution steps will lead to access error
* Returns empty step with message
Copy link
Member

@RichDom2185 RichDom2185 left a comment

Choose a reason for hiding this comment

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

LGTM, thanks a lot! Prof @martin-henz, shall we bump js-slang and quickly deploy the new changes after this?

@coveralls
Copy link

coveralls commented Aug 23, 2023

Pull Request Test Coverage Report for Build 5999207364

  • 3 of 3 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.005%) to 82.679%

Totals Coverage Status
Change from base Build 5995455758: 0.005%
Covered Lines: 10770
Relevant Lines: 12587

💛 - Coveralls

@RichDom2185
Copy link
Member

One minor comment, would it be possible to add test cases to ensure this behaviour?

@sayomaki
Copy link
Contributor Author

Just pushed a simple test for the execution of an empty program (running the test prior to changes from this PR will fail to pass/produce error).

However, it seems that mocking modules have not been fully implemented, so the test for that is not included.

@martin-henz martin-henz merged commit afcff0d into source-academy:master Aug 28, 2023
1 check passed
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.

4 participants