Rewrite MOI.Test

[blegat]

The test names do not follow the style guide, should we rename contlineartest to continuous_linear_test ?
Edit: @odow: hijacking this issue to lay out my plan for the new MOI.Test

Purpose

The purpose of this issue is to track the items that need to be completed in order to rewrite our MOI.Test submodule.

Significant progress and design was started in #1404. But rather than continue that, I'm going to break it into smaller pieces.

Where we are

Our current testing regime is comprehensive, but a bit all over the place.

There's a mix of things like MOI.Test.unittest that wrap a whole lot of tests, and others like MOI.Test.default_status_test that you just need to add. Even the documentation for how to test a solver is complicated (#224)! https://jump.dev/MathOptInterface.jl/dev/submodules/Test/overview/#How-to-test-a-solver

The current design is also bad because it's hard to add new tests.

As evidenced by the documentation: https://jump.dev/MathOptInterface.jl/dev/submodules/Test/overview/#How-to-add-a-test, you need to write a test, then write a test for the test, and then make sure that everything works. It's hard to run a small contingent of tests, and it's hard to decide where to put a new test, and where to put the test of the test.

This also evidenced by the large number of open test issues that aren't getting addressed. These range from #470 (August 2018!) to #1201 (November 2020). If they were easier to add, it would happen quicker!

Naming and visibility of each test is also a problem: #1029. If linear10btest fails, what does that mean?

New design

The basic design is runtests, a single entry point to all tests in MOI.

Instead of breaking down tests by files or dictionaries, tests are normal Julia functions with descriptive names that can be excluded or included by the user.

Here's the code to test the MockOptimizer:

MOI.Test.runtests(
    MOIU.MockOptimizer(MOIU.UniversalFallback(MOIU.Model{Float64}())),
    MOI.Test.Config(basis = true),
    exclude = ["test_nonlinear_"],
)

Much better.

There is also a need for certain tests to modify the model prior to running the test (changing solver parameters/tolerances, for example). That can be achieved by overloading setup_test(::typeof(f), ::MOI.ModelLike, ::MOI.Test.Config) for the particular function f.

TODO

• Rename MOI.Test to MOI.DeprecatedTest [breaking] Deprecate the MOI.Test submodule #1407

• Add core functionality of new MOI.Test, plus documentation (but now actual tests) Add the skeleton of the new MOI.Test submodule #1408 [Test] various fixes to MOI.Test #1428 [Test] change MOI.OPTIMAL to config.optimal_status #1434 [Test] remove supports_optimize in favor of standard exclude #1437

• Migrate existing tests, cleaning an clarifying naming conventions file-by-file

  • UnitTests/attributes [Test] Add test_variable.jl #1410
  • UnitTests/basic_constriant_tests [Test] add test_basic_constraint.jl #1415
  • UnitTests/constraints [Test] add test_constraint.jl #1417
  • UnitTests/modifications [Test] add test_modification.jl #1413
  • UnitTests/objectives [Test] add test_objective.jl #1416 Fix missing line in test_objective.jl #1432
  • UnitTests/solve [Test] add test_solve.jl #1418
  • UnitTests/variables [Test] Add test_variable.jl #1410
  • contconic [Test] add test_conic #1427
  • contlinear [Test] add content from contlinear.jl #1426
  • contquadratic [Test] add test_quadratic.jl #1425
  • intconic [Test] add test_quadratic.jl #1425
  • intlinear [Test] add test_linear.jl #1424
  • modellike [Test] add test_model.jl #1419
  • nlp [Test] add test_nonlinear.jl #1423

• Migrate testing of MOI from DeprecatedTest to the new MOI.Test

  • Bridges
  • FileFormats Migrate FileFormats to new tests #1436
  • Test [Test] Updates to Test/test_model.jl #1438 [Test] updates to Test/test_nonlinear.jl #1439
  • Utilities [Utilities] style updates for UniversalFallback #1440 Update the tests of UniversalFallback to new MOI.Test #1441 [Test] migrate Utilities/model.jl to new MOI.Test #1442 [Utilities] Migrate Utilities/cachingoptimizer to new MOI.Test and fix bugs #1443 [Utilities] migrate print.jl to the new MOI.Test #1444 [Utilities] migrate copy.jl to the new MOI.Test #1445 Refactor DoubleDicts.jl #1446 Migrate tests of Utilities/CleverDicts #1448 Migrate constraints.jl to modular test #1449 Migrate functions.jl to modular test #1450 Migrate tests of lazy_iterators.jl #1451 Migrate tests of sets.jl #1452 Migrate tests of parser.jl #1453 Migrate tests of variables.jl #1455 Migrate tests of mockoptimizer.jl #1456 Migrate tests of matrix_of_constraints.jl #1457

[odow]

I had wanted to do a much larger refactoring of the tests, but it seemed like a lot of breakage for little gain...

[odow]

There are quite a few test-related issues, and it can be hard to make sure you run all tests. At this point I'd say we have a lot of tests, but they could do with a better organization and structure. And that it would make a big quality of life improvement for developers and solver authors.

#224
#307
#1029

It's also a lot of effort to add new tests.

I wonder about having a single Test.runtests function with appropriately named functions that you can include/exclude based on their name.

MOI.Test.runtests(
    optimizer, 
    config, 
    include = [
        "test_linear_",  # name -> startswith(name, "test_linear_")
    ],
    exclude = [
        r"conic",  # name -> occursin(name, r"conic")
    ],
)

[blegat]

I agree, we could do that if we make the tests automatically exclude themself if what's tested is not supported, instead of testing support_constraint and supports at the beginning of each test.
That way, the excluded tests will only indicate failures, an we don't need to update every solver every time we add a new test.

[odow]

I'm working on a PR that I think makes a lot more sense. Stay tuned.

[odow]

@blegat: these get_fallbacks throw an ArgumentError. Shouldn't they throw UnsupportedAttribute instead?

MathOptInterface.jl/src/attributes.jl

Lines 299 to 308 in fe19bbf

	function get_fallback(
	model::ModelLike,
	attr::Union{AbstractModelAttribute,AbstractOptimizerAttribute},
	)
	return throw(
	ArgumentError(
	"$(typeof(model)) does not support getting the attribute $(attr).",
	),
	)
	end

This is causing GLPK to fail the new tests because it doesn't skip unsupported attributes.

[blegat]

UnsupportedAttribute is when the attributes cannot be set and is thrown during set only at the moment.
I guess if is_copyable(attr) && !supports(model, attr) then it might make sense to throw UnsupportedAttribute I have not really understood the use case though.

[odow]

I think it only makes sense to define supports for optional attributes. ListOfModelAttributes etc aren't optional. They're a core part of the API.

[odow]

@blegat any suggestions for how to refactor the bridge testing? They currently take forever to run. And there are some open issues: #820, #750

[blegat]

I think it only makes sense to define supports for optional attributes. ListOfModelAttributes etc aren't optional. They're a core part of the API.

I don't thing there is a well-defined notion of core part of the API. Many ModelLike don't implement ListOfModelAttributesSet

[blegat]

@blegat any suggestions for how to refactor the bridge testing? They currently take forever to run

Moving the @testsets to functions would be a good start, I don't have much ideas of any other improvements.

[odow]

Changing the fallback to throw UnsupportedAttribute isn't so simple, because they actually get captured in a a Test.TestSetException. That means we can't distinguish them from a standard failure like returning the wrong answer.

The work-around would be to change

@test MOI.get(model, MOI.SolverName()) isa AbstractString

to

ret = MOI.get(model, MOI.SolverName())
@test ret isa AbstractString

but that's pretty painful and likely to break.

The better long-term solution is to move away from Base.Test to https://github.com/IainNZ/Pukeko.jl, and modify the @test error to wrap the inner error. We should be able to modify Pukeko to also run each function on a different Julia thread for speed.

[blegat]

I don't understand, we would do that right ?

if MOI.supports(model, MOI.SolverName())
    @test MOI.get(model, MOI.SolverName()) isa AbstractString
end

And you want a way to avoid having to write the if everytime right ?
Maybe something like test_attribute(expected_value, model, attr, args...) that first tests with supports and then do the get ? The isa is more tricky but I assume it's not that common. Otherwise we can have one for isa as well

[odow]

Yeah:

@requires MOI.supports(model, MOI.SolverName())

The other approach is that we already catch and pass on Unsupported or NotAllowed errors. But the mixing with @test obscures this.

Extending supports is still on the cards.

[odow]

Okay! That was an ordeal. I'm closing this issue as complete. The "supports" thing is still unresolved, but I might leave it for now and try updating some solvers. We can open a new issue to track if it becomes a problem.