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

Fix logic error in c4coll_isIndexTrained #2078

Merged
merged 5 commits into from
Jun 25, 2024

Conversation

borrrden
Copy link
Member

It was universally returning true. Also remove the warnings check from the VS test since it makes no sense anymore and is really hard to in the isTrained test now that "untrained" is a warning. Add in a test for the C API to make sure it works correctly.

It was universally returning true.  Also remove the warnings check from the VS test since it makes no sense anymore and is really hard to in the isTrained test now that "untrained" is a warning.  Add in a test for the C API to make sure it works correctly.
@borrrden borrrden requested review from pasin and jianminzhao June 22, 2024 02:22
@pasin
Copy link
Collaborator

pasin commented Jun 22, 2024

Build failed in PR validation.

@cbl-bot
Copy link

cbl-bot commented Jun 25, 2024

Code Coverage Results:

Type Percentage
branches 67.87
functions 79.13
instantiations 35.1
lines 79.19
regions 75.08

@jianminzhao
Copy link
Contributor

CBL-5906
I think this PR does the above. Please confirm.

@borrrden
Copy link
Member Author

Yes, I didn't know there was a ticket for that. I added those checks in.

@snej
Copy link
Collaborator

snej commented Jun 25, 2024

I disagree about removing the warnings check; I think it's useful. And I have a separate PR where I fixed the tests by correctly setting the number of expected warnings. Could you take that out of this PR, and we can decide whether or not to do it?

@borrrden
Copy link
Member Author

borrrden commented Jun 25, 2024

Needing to know how many warnings are logged in order for a test to succeed is really quite obnoxious though. If I take it out of here I need a system of incrementing the expected warnings count by the number of sections since the check only resets at the end of the test....or something like that I can't remember but SECTION seems to mess with it.

Maybe this test is not as bad as it seems with it?  It seems to pass locally now.
@borrrden
Copy link
Member Author

Or maybe I'm hallucinating? I seem to have put the increment in the correct place this time and the test is ok with it.

@borrrden borrrden merged commit 4acfd2b into release/3.2 Jun 25, 2024
7 of 9 checks passed
@borrrden borrrden deleted the fix/index_isTrained_tryCatch branch June 25, 2024 22:12
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.

5 participants