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 failing tests due to updated r2dii.data data #435

Merged
merged 5 commits into from
Jan 15, 2024
Merged

Conversation

jdhoffa
Copy link
Member

@jdhoffa jdhoffa commented Jan 12, 2024

I did a handful of things to ensure the tests are passing as is, mainly to do with adapting tests to the updated format of loanbook_demo:

  • update snapshots
  • update expected col order
  • adapt test to new comp names
  • point to relevant entry

I also removed the call to skip_on_cran, as this should work fine now.

No update to NEWS.md required, as this is not user-facing.

Relates to #432
Closes #434

@jdhoffa jdhoffa requested a review from jacobvjk January 12, 2024 17:45
@jdhoffa
Copy link
Member Author

jdhoffa commented Jan 12, 2024

FYI. @AlexAxthelm

Copy link
Member

@jacobvjk jacobvjk left a comment

Choose a reason for hiding this comment

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

@jdhoffa any idea why a test is failing for some windows checks? the errors seem to point to an issue that we thought was closed: #425

@@ -268,7 +268,7 @@ test_that("works with `min_score = 0` (bug fix)", {
})

test_that("outputs only perfect matches if any (#40 @2diiKlaus)", {
this_name <- "large hdv company three"
this_name <- "Jacob"
Copy link
Member

Choose a reason for hiding this comment

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

lol

Copy link
Member Author

Choose a reason for hiding this comment

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

AHahah it is genuinely a fake company name in the updated data @cjyetman made, ¯_(ツ)_/¯

@jdhoffa
Copy link
Member Author

jdhoffa commented Jan 15, 2024

@jdhoffa any idea why a test is failing for some windows checks? the errors seem to point to an issue that we thought was closed: #425

Hmmm, I do not know why this test is failing... I would say this is not related directly to this PR (updating regressions from r2dii.data), and I would elect to open a new issue to handle this persistent Windows encoding issue.

@jdhoffa
Copy link
Member Author

jdhoffa commented Jan 15, 2024

I have made a new issue in #436. I would suggest we merge this PR, and handle follow-up there.

Copy link
Member

@jacobvjk jacobvjk left a comment

Choose a reason for hiding this comment

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

lgtm

@jdhoffa jdhoffa merged commit 2c1afb6 into main Jan 15, 2024
18 of 22 checks passed
@jdhoffa jdhoffa deleted the 434-fix_tests branch January 15, 2024 13:37
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.

Figure out why 9 tests fail
2 participants