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

Command testing by parsing #66

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Conversation

gubogaer
Copy link
Collaborator

This branch introduces the contextWithParsing function. This context can be used to test intermediate (printed/returned) results that are not saved in an object. This can be done by splitting the student code using comments following a certain pattern. The context function will run the code section by section alternated by the test defined for this section.

This function takes a list of named codeblocks containing the testing function(s) per exercise section. To obtain these sections, the student code is being parsed (split on section titles following the pattern "### sectionname ###"). The section name has to match the named codeblock in the testcases list in order to run the testing functions.
@gubogaer gubogaer requested a review from chvp September 20, 2021 15:21
Copy link
Member

@chvp chvp left a comment

Choose a reason for hiding this comment

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

Please style your code consistently with the rest of the project (space after for and if, space between ) and {, space around ==/!=).

I feel like this doesn't handle mismatches between the exercise's requested sections and the student's provided sections. AFAICT, there is no message when the studnt has superfluous sections, and same for missing? In those cases, a warning to the student also feels appropriate.

Also, IIUC this throws away any code that is not in a section, right? (So all code before the first section, I guess). A student might set up some results/variables there, which could make the rest of the code invalid, even though their solution has correct results when run locally. You could add a warning for that case, but I'd prefer if we executed all submitted code, but just didn't do any tests for a preamble.

README.md Outdated Show resolved Hide resolved
context.R Outdated Show resolved Hide resolved
context.R Outdated Show resolved Hide resolved
context.R Show resolved Hide resolved
gubogaer and others added 3 commits September 24, 2021 11:57
Co-authored-by: Charlotte Van Petegem <charlotte.vanpetegem@ugent.be>
By removing the section titles in the boilertemplate a student could avoid the test associated with this code section
@gubogaer
Copy link
Collaborator Author

gubogaer commented Sep 24, 2021

Please style your code consistently with the rest of the project (space after for and if, space between ) and {, space around ==/!=).

  • code style consistency should be fixed

I feel like this doesn't handle mismatches between the exercise's requested sections and the student's provided sections. AFAICT, there is no message when the studnt has superfluous sections, and same for missing? In those cases, a warning to the student also feels appropriate.

  • superfluous section titles are threated the same as normal comments (skipped)
  • missing sections were a problem, a student could avoid the testing for a section by removing the section titles. This should now be fixed. Removing a section title causes a parsing error

Also, IIUC this throws away any code that is not in a section, right? (So all code before the first section, I guess). A student might set up some results/variables there, which could make the rest of the code invalid, even though their solution has correct results when run locally. You could add a warning for that case, but I'd prefer if we executed all submitted code, but just didn't do any tests for a preamble.

  • This was not the case all student code is being evaluated except when the there are no section titles and no testcases

@gubogaer gubogaer requested a review from chvp September 24, 2021 14:35
@bmesuere
Copy link
Member

@gubogaer It seems like some exercises are already using this? We're getting some internal errors. However, @chvp is ill and I'm abroad so we're unable to fix this today.

@gubogaer
Copy link
Collaborator Author

That's correct luckily I made alternative testing methods which i will be using for the upcoming practica today. But thanks for noticing!

missing_sections <- setdiff(names(testcases), names(codeblocks))
if (length(missing_sections) > 0) {
get_reporter()$add_message(
paste0("Parsing error: could not find rhe following section title(s): \r\n",
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
paste0("Parsing error: could not find rhe following section title(s): \r\n",
paste0("Parsing error: could not find the following section title(s): \n",

if (length(missing_sections) > 0) {
get_reporter()$add_message(
paste0("Parsing error: could not find rhe following section title(s): \r\n",
paste(sapply(missing_sections, function(x) {paste("###", x, "###")}), collapse = ',\r\n'))
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
paste(sapply(missing_sections, function(x) {paste("###", x, "###")}), collapse = ',\r\n'))
paste(sapply(missing_sections, function(x) {paste("###", x, "###")}), collapse = ',\n'))

paste0("Parsing error: could not find rhe following section title(s): \r\n",
paste(sapply(missing_sections, function(x) {paste("###", x, "###")}), collapse = ',\r\n'))
)
get_reporter()$escalate("compilation error")
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure a compilation error makes a lot of sense? Isn't it more logical to add a runtime error (similar to if a function was missing), and still try to evaluate the other testcases?

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