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

Add tests for R/messages.R #89

Merged
merged 13 commits into from
Apr 28, 2023
Merged

Add tests for R/messages.R #89

merged 13 commits into from
Apr 28, 2023

Conversation

averissimo
Copy link
Collaborator

@averissimo averissimo commented Apr 19, 2023

Related issues:

(edit) ℹ️ base branch changed to 82-xportr_length-expand_test_suite

Changes description

  • Corrects bug var_ord_msg() call that was not checking the length of the vector
  • Replaces call to message() with xportr_logger()
  • Simplifies some of the strings that are generated (using glue)
  • Add test cases to cover the R/messages.R file
  • Add mockery dependency to Suggests (optional, but nice to have as it allows to cleanup test output by mocking cli_*() functions)
    • Add test/testthat/helper-mock_bindings.R helper files to contain function re-used in test

image

@bms63
Copy link
Collaborator

bms63 commented Apr 25, 2023

@averissimo no rush - can fix merge conflicts when you have a second please

@bms63 bms63 linked an issue Apr 25, 2023 that may be closed by this pull request
@averissimo averissimo force-pushed the 82-messages-expand_test_suite branch from bc7f327 to af72bf2 Compare April 25, 2023 22:55
@averissimo averissimo changed the base branch from devel to 82-xportr_length-expand_test_suite April 25, 2023 22:55
@averissimo
Copy link
Collaborator Author

@bms63 I've resolved the conflicts and integrated some of the changes from PR #88 (via rebase).

As such, this PR no longer uses mockery and I've changed the base branch to 82-xportr_length-expand_test_suite.

So this PR will either merge to that branch or to devel (depending on which one is merged first 😅)

@averissimo averissimo requested a review from bms63 April 25, 2023 23:05
Base automatically changed from 82-xportr_length-expand_test_suite to devel April 26, 2023 19:47
@bms63 bms63 requested a review from elimillera April 27, 2023 15:48
@@ -135,16 +150,13 @@ label_log <- function(miss_vars, verbose) {
#' @export
var_ord_msg <- function(moved_vars, verbose) {

if (moved_vars > 0) {
if (length(moved_vars) > 0) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

wooo!! what a catch!

@bms63 bms63 merged commit 5b45d1c into devel Apr 28, 2023
@bms63 bms63 deleted the 82-messages-expand_test_suite branch April 28, 2023 20:17
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.

Increase unit test coverage to over 80%
2 participants