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

[R-package] Added unit tests on creating Booster from file or string #2945

Merged
merged 2 commits into from
Apr 8, 2020

Conversation

jameslamb
Copy link
Collaborator

This PR adds unit tests on the code in the R package that handles saving a Booster to file or string and creating a new Booster from a file or string. This brings the unit test coverage on the package from 78.20% to 79.71%.

Importantly, it covers calls from R to C++ functions LGBM_BoosterSaveModelToString_R, LGBM_BoosterCreateFromModelfile_R, and LGBM_BoosterLoadModelFromString_R which were previously not covered (see #2944 for why this is valuable).

This PR does not change the behavior of lgb.load() at all, but it does reorganize the internals to cut out some redundant checks and make the logic a little easier to follow.

This PR also removes an unnecessary step from the docs on generating test coverage in R-package/README.md. Removing the build/ directory is no longer necessary, as of #2909 .

@jameslamb
Copy link
Collaborator Author

Rebased to master, to get the changes from #2954 , so the R test issues should be resolved. That will also fix the weird state of the Travis build, which was a result of the recent Travis outage.

@jameslamb
Copy link
Collaborator Author

gently ping @Laurae2 @guolinke for a review

@jameslamb jameslamb merged commit 91ce04b into microsoft:master Apr 8, 2020
@lock lock bot locked as resolved and limited conversation to collaborators Jun 24, 2020
@jameslamb jameslamb deleted the r/lgb-load-tests branch September 7, 2020 05:26
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants