Add check in unfeasible analyzer: unit tests #1716

sylvlecl · 2023-10-25T08:06:15Z

Description
Adding some unit tests on top of #1689 and of existing code.

Signed-off-by: Sylvain Leclerc <[email protected]>

…analyzer' into feature/add_check_in_unfeasible_analyzer_unit_tests

Signed-off-by: Sylvain Leclerc <[email protected]>

.github/workflows/windows-vcpkg.yml

guilpier-code · 2023-10-31T13:24:45Z

src/tests/src/solver/infeasible-problem-analysis/test-unfeasible-problem-analyzer.cpp

+/*!
+ * Analysis mock, used to assess which step has been run by the analyzer
+ */
+class AnalysisMock : public UnfeasibilityAnalysis


with another C++ unit test library (see GoogleTest, catch 2, CppUTest, ...), we wouldn't need this mock class, and we would need variables like hasRun1, hasPrinted1, ....
We should consider adopting one of these unit tests framework

guilpier-code · 2023-10-31T13:26:09Z

src/tests/src/solver/infeasible-problem-analysis/test-unfeasible-problem-analyzer.cpp

+    analysis.push_back(std::make_unique<AnalysisMock>(true, hasRun2, hasPrinted2));
+    std::unique_ptr<MPSolver> problem(MPSolver::CreateSolver("GLOP"));
+
+    UnfeasiblePbAnalyzer analyzer(std::move(analysis));


Do we need the use of std::move here, as it's used as well in the constructor initialization list ?

Yep, necessary for the compiler, because the vector is move-only (because its content is move-only).
without std::move I guess the compiler tries to perform a copy.

guilpier-code · 2023-10-31T13:42:50Z

src/tests/src/solver/infeasible-problem-analysis/test-unfeasible-problem-analyzer.cpp

+
+BOOST_AUTO_TEST_SUITE(unfeasible_problem_analyzer)
+
+BOOST_AUTO_TEST_CASE(test_problem_analyzer)


The test name is starts with test_ : we know the context is unit test, so this prefix is useless, don't you think ?
Here we could call the test : probe every step of unfeasible analysis ? (may be there's a better name)
Please see next comment.

Overall, I changed the namings according to your input, thank you!
In particular, I indeed removed the test_ prefix, and tried to give a small explanation of the expected behavior ("should" do this).

guilpier-code · 2023-10-31T14:20:08Z

src/tests/src/solver/infeasible-problem-analysis/test-unfeasible-problem-analyzer.cpp

+};
+
+
+BOOST_AUTO_TEST_SUITE(unfeasible_problem_analyzer)


I'm wondering (it's your choice to take this into account) :
We would expect to test an object's behavior (not methods), using it's interface.
We would test simple behaviors, and then more tricky ones.
For example, the names of the tests could be (in the first tests, no need to print a report) :

run_analyzer_with_no_analysis (what should we expect in this case ?)

run_analyzer_with_one_analysis_detecting_a_pb

run_analyzer_with_one_analysis_unable_to_detect_pb

run_analyzer_with_2_analyses__none_detects_a_pb

run_analyzer_with_2_analyses__only_one_detects_pb

run_analyzer_with_2_analyses_detecting_a_pb__only the first_one_is_run

what else ?

About printing reports :
@sylvlecl : we just had a chat about it, run() method should produce a report (for example a vector of std::string), either return by run(), or retrievable with a get method.
This would allow to remove the dependency to the logger.

I left the test as is to test the actual workflow. I agree that for more important piece of codes we should bother to have finer grained tests though.

guilpier-code · 2023-10-31T14:32:39Z

src/tests/src/solver/infeasible-problem-analysis/test-unfeasible-problem-analyzer.cpp

+{
+    ConstraintSlackAnalysis analysis;
+    std::unique_ptr<MPSolver> problem(MPSolver::CreateSolver("GLOP"));
+    const double inf = problem->infinity();


We should not hesitate naming infinity "infinity" instead of "inf", which can be confused with inferior

guilpier-code · 2023-10-31T14:57:59Z

src/tests/src/solver/infeasible-problem-analysis/test-unfeasible-problem-analyzer.cpp

+ *  - Variable 2 must be lesser than -1
+ *  - but if feasible is false, constraint enforces that variable 2 is greater than variable 1 --> infeasible
+ */
+std::unique_ptr<MPSolver> createProblem(const std::string& constraintName, bool feasible)


Calling createProblem() like this createProblem(constraintName, false); is a bit unclear.
A solution could be to have 3 functions :

std::unique_ptr<MPSolver> createSimpleProblem()

void createFeasibleProblem(MPSolver* pb)

void createUnfeasibleProblem(MPSolver* pb)

The last 2 functions would call the first for for the creation a problem with one constraint of 2 variables.

I provided the 2 methods createFeasible and createUnfeasible which just call the initial one.

guilpier-code · 2023-10-31T15:07:37Z

src/tests/src/solver/infeasible-problem-analysis/test-unfeasible-problem-analyzer.cpp

+    "AreaHydroLevel::hour<8>",
+};
+
+BOOST_DATA_TEST_CASE(test_slack_variables_with_unfeasible_constraint, bdata::make(validConstraintNames), constraintName)


Test_ prefix not needed.
This test name does not really reflects what it does.
What it does is :
slack variable analysis detects unfeasibility on many well names constraints.
A bit long, but much clearer.

guilpier-code · 2023-10-31T15:15:44Z

src/tests/src/solver/infeasible-problem-analysis/test-unfeasible-problem-analyzer.cpp

+    BOOST_CHECK(analysis.hasDetectedInfeasibilityCause());
+}
+
+BOOST_AUTO_TEST_CASE(test_slack_variables_with_unfeasible_constraint_name_ignored)


A name could be :
slack variable analysis does not detect unfeasibilty on ill-named constraint

guilpier-code · 2023-10-31T15:20:03Z

src/tests/src/solver/infeasible-problem-analysis/test-unfeasible-problem-analyzer.cpp

+    BOOST_CHECK(!analysis.hasDetectedInfeasibilityCause());
+}
+
+BOOST_DATA_TEST_CASE(test_slack_variables_no_problematic_constraint, bdata::make(validConstraintNames), constraintName)


Do we need to parameterize a test for an analysis that won't detect anything ?

Not very useful indeed, a little redundant with the other test case, I removed the parameterization.

Signed-off-by: Sylvain Leclerc <[email protected]>

sonarqubecloud · 2023-11-03T10:57:34Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

100.0% Coverage
0.0% Duplication

sylvlecl added 2 commits October 25, 2023 09:10

Some unit tests for problem analyzer

64a2a37

Signed-off-by: Sylvain Leclerc <[email protected]>

Fix destructors

f939621

Signed-off-by: Sylvain Leclerc <[email protected]>

sylvlecl requested a review from guilpier-code October 25, 2023 08:06

pull-request-size bot added the size/L label Oct 25, 2023

sylvlecl mentioned this pull request Oct 25, 2023

Unfeasible problem analyzer : add a new check #1689

Merged

9 tasks

sylvlecl added 8 commits October 25, 2023 19:39

Debug windows test

9647b63

Signed-off-by: Sylvain Leclerc <[email protected]>

Try fix

a7b0fd0

Signed-off-by: Sylvain Leclerc <[email protected]>

Restrict unfeasible unit test part

ad2e493

Signed-off-by: Sylvain Leclerc <[email protected]>

Try another fix

489e7c6

Signed-off-by: Sylvain Leclerc <[email protected]>

Final additions

66508dd

Signed-off-by: Sylvain Leclerc <[email protected]>

Merge remote-tracking branch 'origin/feature/add_check_in_unfeasible_…

99e5f16

…analyzer' into feature/add_check_in_unfeasible_analyzer_unit_tests

Post-merge fix

aa9b88f

Signed-off-by: Sylvain Leclerc <[email protected]>

Post-merge fix

90c6a57

Signed-off-by: Sylvain Leclerc <[email protected]>

sylvlecl mentioned this pull request Oct 31, 2023

Mocking library for unit tests #1733

Closed

guilpier-code requested changes Oct 31, 2023

View reviewed changes

Review feedback: mainly naming improvement

561b757

Signed-off-by: Sylvain Leclerc <[email protected]>

sylvlecl merged commit a9c4ad7 into feature/add_check_in_unfeasible_analyzer Nov 3, 2023

sylvlecl deleted the feature/add_check_in_unfeasible_analyzer_unit_tests branch November 3, 2023 12:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add check in unfeasible analyzer: unit tests #1716

Add check in unfeasible analyzer: unit tests #1716

sylvlecl commented Oct 25, 2023

guilpier-code Oct 31, 2023

guilpier-code Oct 31, 2023

sylvlecl Nov 3, 2023

guilpier-code Oct 31, 2023

sylvlecl Nov 3, 2023

guilpier-code Oct 31, 2023

sylvlecl Nov 3, 2023

guilpier-code Oct 31, 2023

sylvlecl Nov 3, 2023

guilpier-code Oct 31, 2023

sylvlecl Nov 3, 2023

guilpier-code Oct 31, 2023

guilpier-code Oct 31, 2023

guilpier-code Oct 31, 2023

sylvlecl Nov 3, 2023

sonarqubecloud bot commented Nov 3, 2023


		BOOST_AUTO_TEST_SUITE(unfeasible_problem_analyzer)

		BOOST_AUTO_TEST_CASE(test_problem_analyzer)

Add check in unfeasible analyzer: unit tests #1716

Add check in unfeasible analyzer: unit tests #1716

Conversation

sylvlecl commented Oct 25, 2023

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

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

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

sonarqubecloud bot commented Nov 3, 2023