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

testing updates after inactivity #121

Merged
merged 18 commits into from
Nov 13, 2023
Merged

testing updates after inactivity #121

merged 18 commits into from
Nov 13, 2023

Conversation

topepo
Copy link
Member

@topepo topepo commented Nov 9, 2023

No description provided.

@hfrick
Copy link
Member

hfrick commented Nov 13, 2023

Whoo, it skips the right tests now!

Status on Matrix: the 1.6-2 binaries for Windows are now on CRAN, so I'm going to lower the minimum version in the skips to that (rather than 1.6-3 as it is currently so that we don't skip these tests for (potentially) too long, waiting for the next release).

@hfrick
Copy link
Member

hfrick commented Nov 13, 2023

Looks like we need the following versions for those three packages (based on an issue comment on the lme4 repo):
Matrix: 1.6-2
RcppEigen: 0.3.3.9.4
lme4: 1.1-35.1

The session info shows that we are getting those versions for lme4 and RcppEigen but not yet for Matrix. That one gets installed in version 1.6-1.1 from CRAN.

So I'm also going to change the skip for lme4 to the current CRAN version 1.1-35.1 and remove the Remote for it (it did not install the dev version based on that anyway).

which got released a few days ago
@hfrick hfrick marked this pull request as ready for review November 13, 2023 13:29
Copy link
Contributor

@simonpcouch simonpcouch left a comment

Choose a reason for hiding this comment

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

We are back on the horse!🐴

@@ -121,19 +121,25 @@ test_that("formula interface can deal with missing values", {
expect_true(is.na(f_pred$.pred[1]))
})

test_that('error traps', {
Copy link
Contributor

Choose a reason for hiding this comment

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

Appreciating how this was split up. :)

Error <rlang_error>
`data` must only contain numeric columns.
Condition
Error in `recompose()`:
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this a regression? Not sure if this is a change in rlang or our own, but if this seems like a step backward I say we file an issue.

Copy link
Contributor

Choose a reason for hiding this comment

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

edit: referring to the reference to recompose() newly showing up.

Error <rlang_error>
For the glmnet engine, `penalty` must be a single number (or a value of `tune()`).
Condition
Error in `.check_glmnet_penalty_fit()`:
Copy link
Contributor

Choose a reason for hiding this comment

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

Same thought re: new call output.

tests/testthat/test-parsnip-model-formula.R Show resolved Hide resolved
This was referenced Nov 13, 2023
@hfrick
Copy link
Member

hfrick commented Nov 13, 2023

Re error call:
If it's coming from hardhat: that hasn't been touched since before extratests broke down. If it's coming from parsnip: that totally needs various updates on how error calls are handled. So I think we'd pass over this once we do parsnip errors - which is one of those "epic" issues. So maybe not necessary to open issues specifically for those two error calls? But happy to do so if you think that's the better approach!

@simonpcouch
Copy link
Contributor

That makes sense, thanks! :)

@hfrick
Copy link
Member

hfrick commented Nov 13, 2023

I shall merge then! 🎉

@hfrick hfrick merged commit 04ee2d1 into main Nov 13, 2023
5 checks passed
@hfrick hfrick deleted the 2023-11-snapshot-update branch November 13, 2023 15:00
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.

3 participants