Skip to content

Conversation

joaquimg
Copy link
Member

@joaquimg joaquimg commented Nov 7, 2021

When tests require a number because of a duplicate name they were using ii and 2. I kept the latter because it is clearer.
We should highlight this in docs?

@blegat blegat requested a review from odow November 7, 2021 13:00
Copy link
Member

@odow odow left a comment

Choose a reason for hiding this comment

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

Okay for now, but we need to come up with some documented rules for when we will add new tests or modify existing tests. This will cause solver tests to fail if the previously excluded the full name.

Is it okay to break tests of solvers in a patch/minor/major release of MOI?

@joaquimg
Copy link
Member Author

joaquimg commented Nov 8, 2021

Is it okay to break tests of solvers in a patch/minor/major release of MOI?

If the test is wrong, we should fix it even in the patch.
If a test tests existing functionality it could go in a patch too.
These two cases would break solver in a patch just as much a bug fix could break them.

Renaming is a bit more complicated indeed.
We could check if some entry of the excluded list is never used, so the solver has a chance to clean up.

In this special case, we can leave the renaming for 0.11.

We certainly need clear rules for capitalization (use if and only if it refers to a struct) and for numbering.

@odow odow added the breaking label Nov 9, 2021
Copy link
Member

@odow odow left a comment

Choose a reason for hiding this comment

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

I think this is technically breaking, but I'm okay putting it in a patch release. This will only break tests of solvers, not their user-facing behavior.

Copy link
Member

@blegat blegat left a comment

Choose a reason for hiding this comment

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

Agreed to have this in a patch release

@odow odow merged commit 2e969ef into master Nov 9, 2021
@odow odow deleted the jg/test_names branch November 9, 2021 23:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

3 participants