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 expect_s3_class() tests #182

Merged
merged 11 commits into from
Dec 18, 2024
Merged

fix expect_s3_class() tests #182

merged 11 commits into from
Dec 18, 2024

Conversation

zkamvar
Copy link
Member

@zkamvar zkamvar commented Dec 13, 2024

I believe this might be good to go through commit by commit.

As described in #180, I've gone through and I've updated the expectations for expect_s3_class() to actually test for the classes we expect. I used the following strategy:

  1. if an expectation has exact = TRUE, I leave it alone
  2. otherwise, I remove all but the first class

The reason why I did not append exact = TRUE to every test is because there were several tests that did not contain the "hub_check" class. Moreover, the vast majority of these tests were checking if there was a successful check or not, which the top-level check_success, check_failure, etc. gives us.

There is one check that is skipped pending further investigation and can be fixed in another pull request (issue: #183) (see #180 (comment) and #180 (comment) for the rationale).

This will fix #180

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
In 8c2d46d, a missing metadata file was
downgraded to a `check_failure`, but this check continued to pass
because it still inherited from `rlang_error`, `error`, and `condition`
One check was skipped due to changed expectations within the last year:
#180 (comment)
This check had erroneously expected a `check_success` from the
check_tbl_spl_compound_tid, but in reality, it was an error as it
was the exact same incantation as the command that created the
`error_check` object above.
Copy link

github-actions bot commented Dec 13, 2024

Copy link
Member

@annakrystalli annakrystalli left a comment

Choose a reason for hiding this comment

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

Looks good to me and thanks for sorting this out! I've added one suggestion to correct a test that should be running a different test (rather than the result being wrong). Other than that, it's good to go from my end.

tests/testthat/test-check_tbl_spl_compound_tid.R Outdated Show resolved Hide resolved
tests/testthat/test-check_tbl_spl_compound_tid.R Outdated Show resolved Hide resolved
@@ -20,7 +20,7 @@ test_that("check_metadata_file_exists works", {
hub_path = hub_path,
file_path = "random-model/2022-10-01-random-model.csv"
),
c("check_error", "rlang_error", "error", "condition")
Copy link
Member

Choose a reason for hiding this comment

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

Good catch!

When correcting s3 class tests, @zkamvar discovered a test that was actually failing when it should in fact be passing. The reason was an error in the test files which included trailing zeros in quantiles. Once that was corrected the test passed.
While it has no actual effect on tests and is only an internal test file, it goes against the best practice we advocate: https://hubverse.io/en/latest/quickstart-hub-admin/tasks-config.html#setting-up-quantile
@annakrystalli
Copy link
Member

Note as well that I fixed the skipped test and opened a PR to here #184

zkamvar and others added 2 commits December 18, 2024 08:41
Co-authored-by: Anna Krystalli <annakrystalli@googlemail.com>
…/183

Fix errors in test model output file
@zkamvar
Copy link
Member Author

zkamvar commented Dec 18, 2024

Thank you for the review and the corrected test, Anna!

@zkamvar zkamvar merged commit def750c into main Dec 18, 2024
8 checks passed
@zkamvar zkamvar deleted the znk/s3-class-tests/180 branch December 18, 2024 16:56
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.

add exact = TRUE or reduce expected classes for expect_s3_class()
2 participants