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

Better hygiene around S3 methods in tests #740

Merged
merged 1 commit into from
Mar 12, 2025

Conversation

jennybc
Copy link
Member

@jennybc jennybc commented Mar 12, 2025

While working on something else, I noticed a flaky test:

failures:

---- variables::variable::tests::test_matrix_display stdout ----

thread 'variables::variable::tests::test_matrix_display' panicked at crates/ark/src/variables/variable.rs:2133:13:
assertion `left == right` failed
  left: "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"
 right: "[1 row x 1 column] <foo>"
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace


failures:
    variables::variable::tests::test_matrix_display

Discussed with @DavisVaughan who offered some very helpful analysis: this comes from two different tests being run in the same R session ?in parallel or in some random order?. Both have assertions that tickle the ark_positron_variable_display_value method for a toy "foo" class and they can crosstalk with each other.

Again at @DavisVaughan's suggestion, I am introducing and using a method to unregister an S3 method in ark's method table.

I have very low-tech empirical proof that it's working. Before and after this PR, I ran cargo test --package ark --lib variables::variable::tests 10 times. Before, I saw 2 failures (the exact one shown above) and 8 passes. After, I see 10 passes.

#' @export
.ark.register_method <- function(generic, class, method) {
# Check if the caller is an allowed package
# check if the calling package is allowed to touch the methods table
Copy link
Member Author

Choose a reason for hiding this comment

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

There was just so much duplication between the register/unregister that I think these helpers might be nice?

@jennybc jennybc force-pushed the bugfix/unregister-S3-methods-in-tests branch from 15f9372 to a9e5609 Compare March 12, 2025 18:15
@jennybc jennybc requested review from dfalbel and lionel- March 12, 2025 18:15
Copy link
Contributor

@dfalbel dfalbel left a comment

Choose a reason for hiding this comment

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

LGTM!

?in parallel or in some random order?

They don't run in parallel as only a single r_task can be running due to the mutex. But they can definitely run in a undeterministic order, depending on which r_task is able to acquire the lock first.

@jennybc jennybc merged commit 676b7ba into main Mar 12, 2025
6 checks passed
@jennybc jennybc deleted the bugfix/unregister-S3-methods-in-tests branch March 12, 2025 21:22
@github-actions github-actions bot locked and limited conversation to collaborators Mar 12, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants