RFC: unit tests #329

odow · 2018-04-27T04:29:35Z

This PR adds some atomic tests that aim to minimally test each expected feature in addition to the full end-to-end tests in contlinear.jl etc.

It has already found a few bugs in LinQuadOptInterface that weren't following the API to the letter.

Bug fixes to existing tests

If we delete the variable as it was, we can't add the constraints.

Questions

~~- Should we enforce a default objective sense?~~ Raised as separate issue #335

Other

Closes #244

codecov-io · 2018-04-27T04:36:21Z

Codecov Report

Merging #329 into master will decrease coverage by 1.68%.
The diff coverage is 31.65%.

@@            Coverage Diff             @@
##           master     #329      +/-   ##
==========================================
- Coverage   96.47%   94.79%   -1.69%     
==========================================
  Files          31       34       +3     
  Lines        4537     4837     +300     
==========================================
+ Hits         4377     4585     +208     
- Misses        160      252      +92

Impacted Files	Coverage Δ
src/MathOptInterface.jl	`100% <ø> (ø)`	⬆️
src/Test/UnitTests/constraints.jl	`0% <0%> (ø)`
src/Test/modellike.jl	`100% <100%> (ø)`	⬆️
src/Test/UnitTests/variables.jl	`25% <25%> (ø)`
src/Test/UnitTests/objectives.jl	`50% <50%> (ø)`
src/indextypes.jl	`62.5% <50%> (ø)`	⬆️
src/Utilities/parser.jl	`97.59% <0%> (-0.86%)`	⬇️
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d9295f1...026c0e3. Read the comment docs.

mlubin · 2018-04-27T13:06:58Z

Another reason to split these tests apart.

blegat · 2018-04-28T16:08:16Z

src/Test/atomic_tests.jl

+"""
+    This function tests adding a single variable.
+"""
+function add_variable(model::MOI.ModelLike, config::TestConfig)


It would be more consistent with the rest of the tests to call it addvariabletest

blegat · 2018-04-28T16:09:34Z

Should we enforce a default objective sense?

This is related to the question : Should a solver error if optimize! is called and ObjectiveSense has not been set or assume it is FeasibilitySense ?

odow · 2018-04-29T02:56:07Z

I guess we can just leave each solver to have a default. Otherwise despite the backend having a default sense, every MOIWrapper will have to store a flag indicating whether the sense has been set.

odow · 2018-04-29T03:12:46Z

I had to exclude a few of the atomic tests since MockOptimizer doesn't implement names.

blegat · 2018-04-29T09:22:42Z

every MOIWrapper will have to store a flag indicating whether the sense has been set

Yes, that is the reason, we stay vague on this issue. ObjectiveSense will not appear in ListOfModelAttributesSet so it won't be used on copy! hence the value the model has is not critical.

blegat · 2018-04-29T09:49:20Z

I had to exclude a few of the atomic tests since MockOptimizer doesn't implement names.

Should be fixed in #336 :)

mlubin · 2018-04-30T12:46:49Z

src/Test/AtomicTests/constraints.jl

+    Test getting constraints by name.
+"""
+function getconstraint(model::MOI.ModelLike, config::TestConfig)
+    MOI.empty!(model)


It's weird that we have to call MOI.empty! at the beginning of every test. This should happen somewhere else.

I agree. Maybe it should go here?
https://github.com/JuliaOpt/MathOptInterface.jl/blob/84c5b19fa7cea292b715f6e02658bf977db08134/src/Test/config.jl#L42-L43

That can be a job for a different PR.

mlubin · 2018-04-30T12:49:29Z

src/Test/AtomicTests/objectives.jl

+    MOI.empty!(model)
+    @test MOI.isempty(model)
+    MOIU.loadfromstring!(model,"""
+        variables: x


This is a great use of loadfromstring!

mlubin · 2018-04-30T12:50:56Z

src/Test/AtomicTests/objectives.jl

+"""
+    Test constant in objective.
+"""
+function constant_obj(model::MOI.ModelLike, config::TestConfig)


If the test calls optimize!, I'd propose to have "solve" somewhere in the name, like solve_constant_obj. You could easily think that constant_obj would just set and get the objective attribute without solving.

mlubin · 2018-04-30T12:53:10Z

src/Test/AtomicTests/variables.jl

+atomictests["variablenames"]    = variablenames
+
+"""
+    Test the setting of an upper bound


Another example where "solve" in the name would be informative, e.g., solve_with_upperbound.

mlubin

This is a big improvement for the tests. "Atomic" is a bit of a misnomer because some of the tests do more than one thing (e.g., delete_variables). You could call these small tests or unit tests.

blegat · 2018-05-01T07:47:43Z

test/Test/unit.jl

+        "solve_with_lowerbound",
+        "solve_with_upperbound",
+        "getconstraint",
+        "getvariable",


getconstraint, getvariable and variablenames should work now that #336 is merged.
You can test the solve tests with mock_optimize
https://github.com/JuliaOpt/MathOptInterface.jl/blob/da6e0910d382ce83b047991d0ba6c6bfece4ddc1/src/Utilities/mockoptimizer.jl#L271-L281
See https://github.com/JuliaOpt/MathOptInterface.jl/blob/master/test/contlinear.jl for example for using mock_optimize

I'm not sure that the mock optimizer implements the names correctly. I got some errors. I'm going to merge this as is, the outstanding tests can be addressed in later PR's.

mlubin · 2018-05-01T13:22:32Z

src/Test/UnitTests/variables.jl

+unittests["variablenames"]    = variablenames
+
+"""
+    Test the setting of an upper bound


Also update the docstring to mention that the test calls optimize!.

mlubin · 2018-05-01T13:22:38Z

src/Test/UnitTests/variables.jl

+unittests["solve_with_upperbound"]    = solve_with_upperbound
+
+"""
+    Test the setting of an lower bound


Change order of tests

faee6d9

odow mentioned this pull request Apr 27, 2018

DNMY: MOI jump-dev/Gurobi.jl#125

Merged

12 tasks

blegat approved these changes Apr 27, 2018

View reviewed changes

Start some atomic tests

fb97f1f

odow changed the title ~~Change order of tests~~ Some more atomic tests Apr 28, 2018

odow added 2 commits April 28, 2018 19:30

more tests

10f8cfd

Objective tests

e89135c

odow mentioned this pull request Apr 28, 2018

DNMY: MOI jump-dev/GLPK.jl#53

Merged

14 tasks

Test setting variable name

b04d858

blegat reviewed Apr 28, 2018

View reviewed changes

reorganize and add test for SingleVariable

710510c

Add atomic to tests

c387d55

odow mentioned this pull request Apr 29, 2018

Default ObjectiveSense #335

Closed

odow changed the title ~~Some more atomic tests~~ RFC: some more atomic tests Apr 29, 2018

mlubin reviewed Apr 30, 2018

View reviewed changes

Address mlubin's comments

496b98d

odow changed the title ~~RFC: some more atomic tests~~ RFC: unit tests Apr 30, 2018

Update filename

218d5bc

blegat reviewed May 1, 2018

View reviewed changes

mlubin approved these changes May 1, 2018

View reviewed changes

Update doc strings

026c0e3

odow merged commit cd3115d into master May 1, 2018

odow deleted the odow/grb branch May 1, 2018 21:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RFC: unit tests #329

RFC: unit tests #329

odow commented Apr 27, 2018 •

edited

Loading

codecov-io commented Apr 27, 2018 •

edited

Loading

mlubin commented Apr 27, 2018

blegat Apr 28, 2018

blegat commented Apr 28, 2018

odow commented Apr 29, 2018

odow commented Apr 29, 2018

blegat commented Apr 29, 2018

blegat commented Apr 29, 2018

mlubin Apr 30, 2018

odow Apr 30, 2018

mlubin Apr 30, 2018

mlubin Apr 30, 2018

mlubin Apr 30, 2018

mlubin left a comment

blegat May 1, 2018 •

edited

Loading

odow May 1, 2018

mlubin May 1, 2018

mlubin May 1, 2018

RFC: unit tests #329

RFC: unit tests #329

Conversation

odow commented Apr 27, 2018 • edited Loading

Bug fixes to existing tests

Questions

Other

codecov-io commented Apr 27, 2018 • edited Loading

Codecov Report

mlubin commented Apr 27, 2018

Choose a reason for hiding this comment

blegat commented Apr 28, 2018

odow commented Apr 29, 2018

odow commented Apr 29, 2018

blegat commented Apr 29, 2018

blegat commented Apr 29, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mlubin left a comment

Choose a reason for hiding this comment

blegat May 1, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

odow commented Apr 27, 2018 •

edited

Loading

codecov-io commented Apr 27, 2018 •

edited

Loading

blegat May 1, 2018 •

edited

Loading