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 tests for upcoming checkmate #134

Merged
merged 2 commits into from
Oct 26, 2021
Merged

Fix tests for upcoming checkmate #134

merged 2 commits into from
Oct 26, 2021

Conversation

mllg
Copy link
Contributor

@mllg mllg commented Oct 25, 2021

The development of checkmate has slowed down a bit in the last few months, which is usually a sign that the package is now very mature. Of course, there is still some work to be done, and for the next version some error messages should be improved, among other things.

However, this is only possible if the reverse dependencies do not unit test for the exact error message.

As an example:

x = "a"
assert_character(x, min.chars = 2)

used to throw the error message

Assertion on 'x' failed: All elements must have at least 2 characters.

In the development version of checkmate, I've added the information about which element of the vector is responsible for the assertion to be triggered, and changed the error message to

Assertion on 'x' failed: All elements must have at least 2 characters, but element 1 has 1 character.

To overcome this problem, it is generally a good idea in unit tests to not grep on complete error messages, but restrict the expectation to the relevant part. In the above example, "2 characters" or "at least 2 characters" would probably have been sufficient.

This PR addresses failing tests in set expectations, where the error message now gives more precise information about which elements are violating the expectation (instead of having the user parse and compare two printed sets).

@MalteKurz
Copy link
Member

Thanks a lot for the updates and the recommendations. I will review asap.

@MalteKurz MalteKurz self-requested a review October 25, 2021 11:56
@MalteKurz MalteKurz self-assigned this Oct 25, 2021
@MalteKurz
Copy link
Member

@mllg Thanks a lot for the heads-up and for directly fixing things.

  • I did trigger unit tests with checkmate from https://github.com/mllg/checkmate in branch master. Then I still got four failing tests, see https://github.com/DoubleML/doubleml-for-r/runs/3998091580?check_suite_focus=true. I think the error messages where adapted even a bit more. E.g. "Assertion on 'levels\\(data\\[\\[target\\]\\])' failed: Must be equal to set \\{'0','1'\\}" became "Assertion on 'levels(data[[target]])' failed: Must be a permutation of set {'0','1'}, but has extra elements {'1.5'}.". Therefore, simply shortening the regular expression was not enough. I did fix this in 65a827c.
  • I fully agree that checking the entire error messages is too much and we should get more flexible in this regards by using regular expressions. I will try to adhere to this in the future.

I have one final question: Will there be further adaptions to checkmate or will the coming release be in line with the current master on github?

P.S.: We soon anyways need to release a new version of DoubleML to CRAN due to failing unit tests on solaris. Therefore, I expect to be able to release the changes from this PR rather soon to CRAN.

@mllg
Copy link
Contributor Author

mllg commented Oct 26, 2021

I have one final question: Will there be further adaptions to checkmate or will the coming release be in line with the current master on github?

The current state in the master branch is the release candidate, but I'm not sure when this will be uploaded as I'm still fighting with multiple broken reverse dependencies.
If I find bugs, I will fix them, but I will not introduce any more potentially breaking changes.

@MalteKurz
Copy link
Member

MalteKurz commented Oct 26, 2021

I have one final question: Will there be further adaptions to checkmate or will the coming release be in line with the current master on github?

The current state in the master branch is the release candidate, but I'm not sure when this will be uploaded as I'm still fighting with multiple broken reverse dependencies. If I find bugs, I will fix them, but I will not introduce any more potentially breaking changes.

Thanks for the clarification 👍. No worries with regards to breaking changes, if they are needed we can adapt accordingly.

@MalteKurz MalteKurz merged commit 52301d2 into DoubleML:master Oct 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants