Skip to content

Conversation

topepo
Copy link
Member

@topepo topepo commented Aug 6, 2025

The survival tests had many ptypes and vectors of column names that failed once we changed the column order.

We might want to make a time-based skip to amend these once we are nearly done to make them sensitive to order, but I don't think that now is the time for that action. Alternatively, we can augment with some specific tests that lock down specific column orderings.

@topepo topepo requested review from EmilHvitfeldt and hfrick August 6, 2025 17:48
@EmilHvitfeldt EmilHvitfeldt changed the title making some tests insensitive to column order making some tests insensitive to column order, survival tests Aug 6, 2025
Copy link
Member

@EmilHvitfeldt EmilHvitfeldt left a comment

Choose a reason for hiding this comment

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

looks good to me. I added a couple more expect_named() to these files. I didn't set ignore.order as they only checked for 1 column tibbles

Copy link
Member

@hfrick hfrick left a comment

Choose a reason for hiding this comment

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

Re revisiting: I've updated #244 to remove the ingore.order = TRUE again and then go over fixing the column order 👍

@@ -82,7 +82,7 @@ test_that("autoplot-ting survival models with static metric", {
name = character(0),
value = numeric(0)
)
expect_equal(exp_data_ptype, stc_marginal$data[0,])
expect_ptype(stc_marginal$data, exp_data_ptype)
Copy link
Member

Choose a reason for hiding this comment

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

I like the side effect of this finally going in the order of "actual, expected" :)

@EmilHvitfeldt EmilHvitfeldt marked this pull request as ready for review August 13, 2025 22:16
@EmilHvitfeldt EmilHvitfeldt merged commit 2e158fd into main Aug 13, 2025
0 of 5 checks passed
@EmilHvitfeldt EmilHvitfeldt deleted the naming-insensitivity branch August 13, 2025 22: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.

3 participants