✔️ Linting Passed

All linting checks passed. Your pull request is in excellent shape! ☀️

_{Generated for commit: 9d8b93b. Link to the linter CI: here}

Continuing the discussion from #32351 about testing here, as I think it's relevant for this PR.

@cakedev0 said:

I personally love tests with many random inputs. Toy examples should only be used for debugging IMO. It's still good to have them as tests, as it makes it easy to debug regressions. But it's really not enough to build confidence.

@ogrisel said:

Unfortunately, such randomized tests are often costly to run so we need to strike a balance. Ideally, individual tests should last no longer than 1 s on a usual CI runner.
Slow test suites makes it painful to run the tests locally or wait for the CI to complete on a PR. It would further make the release process more horrible than it is, given the number of Python versions and architecture combination that we support.

My answer:

I have that in mind, I don't think randomized tests have to be slow, I try to hit the proper balance, but yes it's not easy.

Currently, all the tests of sklearn/tree run in ~6s, while the tests of sklearn/ensemble/ run in ~100s. Given that a lot of things in sklearn/ensemble/ rely on sklearn/tree/, I think it's reasonable to spend a bit more time in testing it.

Note that the test in this PR takes only ~2s for 20 generated variants.

Thanks for the review and the great comments suggestions!

We could also test for deeper trees and recursively check [...]

I don't think it's worth the effort: I think this part (tree-building logic) is low-risk and isn't going to change soon.

While for the splitting part, there are a lot of changes/potential future changes (my missing-values refactor, categorical, sparse + NaNs, binning, other optimizations?). Plus there was quite a few small edge-case bugs. So we really needed such a test.

Note: The ~20 generated tests run in ~1s, so it's a very reasonable load on the CI ^^

I don't think it's worth the effort

I agree.