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

Protect options() from modifications by student or checking code #542

Merged
merged 16 commits into from
Jun 23, 2021

Conversation

rossellhayes
Copy link
Contributor

This modifies evaluate_exercise() to capture the state of options() when the tutorial is first loaded. This snapshot is restored each time an exercise is run. This allows tutorial authors to still set options in the setup chunk. Students can modify options within a chunk, but these options are not carried over to later chunks or subsequent runs of the same chunk.

@gadenbuie gadenbuie self-requested a review June 17, 2021 20:37
@rossellhayes
Copy link
Contributor Author

Are there any other things besides options that need to be protected in this way?

@rossellhayes rossellhayes marked this pull request as ready for review June 20, 2021 23:28
Copy link
Member

@gadenbuie gadenbuie left a comment

Choose a reason for hiding this comment

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

Looks great! Just a few comments to simplify the code and improve readability of the tests

R/exercise.R Outdated Show resolved Hide resolved
tests/testthat/test-exercise.R Outdated Show resolved Hide resolved
sandbox/options/options-test.Rmd Outdated Show resolved Hide resolved
@rossellhayes
Copy link
Contributor Author

I've found that the same issue with options() also applies to environment variables. This is addressed in 15358af.

R/exercise.R Outdated Show resolved Hide resolved
Copy link
Member

@gadenbuie gadenbuie left a comment

Choose a reason for hiding this comment

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

One more little update (see #542 (comment)) and then we'll be good to go. Can you also please add a NEWS item for this PR? Thank you!

@gadenbuie
Copy link
Member

@rossellhayes I added two tests that also check that the checking code doesn't end up modifying the options or env vars. Do you think we've covered all of our bases in the tests?

@rossellhayes
Copy link
Contributor Author

Good thinking! That all seems good to me, I can't think of anything that's missing

@gadenbuie gadenbuie changed the title Protect options() from modifications by students Protect options() from modifications by student or checking code Jun 23, 2021
@gadenbuie gadenbuie merged commit 9ddde0a into rstudio:master Jun 23, 2021
@rossellhayes rossellhayes deleted the protect-options branch June 24, 2021 03:26
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.

2 participants