Skip to content

[Test] add version_added to better control MOI.Test.runtests#1662

Merged
odow merged 4 commits intomasterfrom
od/test-limit
Nov 18, 2021
Merged

[Test] add version_added to better control MOI.Test.runtests#1662
odow merged 4 commits intomasterfrom
od/test-limit

Conversation

@odow
Copy link
Copy Markdown
Member

@odow odow commented Nov 17, 2021

Closes #1661

Not sold on the exclude_tests_after name; open to bike shed.

I think with this design it lets us be much more liberal with adding new (potentially breaking) tests, and we won't have this constant fight about updating solvers. Then, solver authors can periodically increment this bound and deal with the fallout. In a post-1.0 world of MOI, nothing of their existing code should break, so this is really just a catch on adding new features.

@odow odow added the Submodule: Tests About the Tests submodule label Nov 17, 2021
@odow odow requested review from blegat and joaquimg November 17, 2021 23:38
@joaquimg
Copy link
Copy Markdown
Member

I like 'version_added'. Would be great to have 'include_tests_before' falling back to the version defined in the Project file of the caller similar to the suggestion in #1661 (comment)

@odow
Copy link
Copy Markdown
Member Author

odow commented Nov 18, 2021

I think the default needs to be "test everything unless otherwise instructed." You should have to opt-out of testing, rather than opt-in.

@joaquimg
Copy link
Copy Markdown
Member

Fair enough, in that case the suggestion would be adding a method: moi_version()

@joaquimg
Copy link
Copy Markdown
Member

Maybe required_moi_version("PKG_NAME") so that user might use this default that only require updating the project file.
Also, it would be useful to check if the required version in deps is greater than the version selected in exclude_tests_after, which could even be an error

@odow
Copy link
Copy Markdown
Member Author

odow commented Nov 18, 2021

in that case the suggestion would be adding a method: moi_version()

I don't think I understand the suggestion. This isn't intended for casual users. It's for authors of solver packages who should have a strong understanding of which version of MOI that they are using. I expect it to be unset (default; run all tests), or the version explicitly hard-coded. I don't think we should provide anything other than that.

@joaquimg
Copy link
Copy Markdown
Member

I imagine solvers selecting a version for tests and then updating the MOI version (to yet another version). Hence, new functionality that came in the selected version of MOI (of the project.toml) would be untested.
Having to select versions of MOI in two parts of the code has the potential for bugs.

Also, I imagine many authors using a lot of this functionality since it is very annoying to have tests breaking just because MOI was updated.

I am very in favor of required_moi_version("PKG_NAME").
But I would be fine with not having such method and internally comparing the hard-coded version with the version in project.toml to guarantee that version in project <= version of tests.

@odow
Copy link
Copy Markdown
Member Author

odow commented Nov 18, 2021

People can hard-code an upper bound into the Project.toml files if they want.

How about we merge this as-is, wait to see how ergonomic it is for packages, and then make a decision to add something later if it's a real pain?

@blegat
Copy link
Copy Markdown
Member

blegat commented Nov 18, 2021

I like this feature. Maybe adding a warning if the option is set ? So that package author remember that they may be missing new tests.

@odow
Copy link
Copy Markdown
Member Author

odow commented Nov 18, 2021

No one looks at the warnings in the logs. Let's merge as-is and see how it plays out.

Adding a warning is non-breaking, and adding clever tools to detect the version is also non-breaking if we make it exclude_tests_after = :auto.

@odow odow merged commit 4d7c42d into master Nov 18, 2021
@odow odow deleted the od/test-limit branch November 18, 2021 23:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Submodule: Tests About the Tests submodule

Development

Successfully merging this pull request may close these issues.

Add a way to add tests without breaking solvers

3 participants