Enables NoCalibratorLinearSvmTest #232
Conversation
| } | ||
|
|
||
| /// <summary> | ||
| ///A test for PAV calibrators | ||
| ///</summary> | ||
| [Fact(Skip = "Need CoreTLC specific baseline update")] |
There was a problem hiding this comment.
Is there a reason this one is still skipped?
There was a problem hiding this comment.
They are getting enabled in #233
There was a problem hiding this comment.
There was a problem hiding this comment.
Hi @Anipik , so is what you're trying to do here, attempt to address the comment in #78 where @Ivanidzo4ka suggested a 471 file PR was hard to review, and asked to break these things into manageable chunks? If the goal is to make an easy-to-review set of PRs, I'm not sure a series of "nested" PRs is really helping, since, say, #233 has everything anyway, and we're made to understand #233 in the context of #232, but of course we can't actually do that unless we find the commit ids and run the diff ourselves.
Do I understand the situation with what you're trying to do with these PRs? If I do, I'm inclined to accept your PRs #233, #231, #228, and reject your #232, #230, #229, #227. (This of course assuming I've correctly interpreted how this ecosystem of PRs has been structured. Let me know if I haven't.) Sound good?
General thoughts: As near as I know there is no policy expressed anywhere against enabling multiple tests at once. I think the problem is that #78 was so large that it could not be effectively reviewed -- indeed the github web UI seemed to have no idea what to do with it, and Codeflow was similarly flummuxed. This latest series went in the opposite direction to the point that it is also hard to review. An intermediate stage between all-at-once and one-at-a-time would probably be more ideal. That is, #233 and #230 and #228, on their own seem like manageable easy-to-review chunks, so maybe we ought to structure the future PRs in that fashion.
Anyway, let me know.
There was a problem hiding this comment.
@TomFinley I tried to enable one test at a time. Some of the existing tests are testing multiple models so i split them in the PR eg #229 #232
I should have waited to create a PR for these new tests eg. #230 and #231 depends upon #229
#227 #228 #231 #233 needs to merged. #230 would need to be rebased after that.
There was a problem hiding this comment.
OK. Sorry to hear this. In that case please cancel them all, or rephrase them as independent PRs. As near as I can see you will want three PRs. One for FastTree, one for AP, and one for I guess the calibration.
So let's think about what is actually happening here. These tests are, what, about three lines long each, and run independently of each other. The idea that the PRs for this somehow "need" to be nested does not make sense to me.
Let's go with this: goal should be to phrase PRs that are easy to review. Not too large. But large enough so I don't have to look a 7 separate PRs, each of which has perhaps two lines different between them and are all touching the same lines, just to figure out what is going on.
ghost
locked as resolved and limited conversation to collaborators
Related to #78
Test is enabled and corresponding Zbaseline files are added
cc @shauheen @Ivanidzo4ka @eerhardt @danmosemsft @codemzs