conjure-cp / conjure-oxide

Mozilla Public License 2.0
9 stars 16 forks source link

Refactor the integration testing code #523

Open ozgurakgun opened 4 days ago

ozgurakgun commented 4 days ago

You can start by writing a description @YehorBoiar :)

YehorBoiar commented 2 days ago

Problem Description:

In PR #421, tests are incorrectly passing even though there are mismatches between Conjure solutions and Conjure-Oxide solutions for some models. This issue arises because when running tests with ACCEPT=true (i.e., ACCEPT=true cargo test), the expected solution files are automatically rewritten with the generated solutions, even if they don’t match Conjure solutions. As a result, the next time the tests run, they always pass since the test framework compares the newly generated solutions with the updated expected files (which are identical).

Solution:

To resolve this, the key is to avoid overwriting the expected files when the ACCEPT=true flag is set, unless the generated solutions match the Conjure solutions.

Refactoring Approach:

  1. When ACCEPT=true:

    • Generate all files (parse, rewrite, solve) as usual.
    • Only assert that the generated solutions match the Conjure solutions. Parsed, rewrite, and trace are assumed to be correct.
    • If the solutions match, rewrite the expected files (i.e., save_model_json and save_minion_solutions_json).
    • If the solutions don’t match, fail the test without rewriting anything.
  2. When ACCEPT=false:

    • Generate all files (parse, rewrite, solve) as usual.
    • Assert the generated files against the expected solutions.

Implementation Details:

The integration_test_inner function will be refactored to:

YehorBoiar commented 2 days ago

@ozgurakgun Looks good?

ozgurakgun commented 2 days ago

I stopped reading at "implementation details".

reads a bit like gpt (mostly correct, well structured, but slightly incorrect in a few important ways), which is not a problem per se, but make sure you agree with what it says. I would just remove the "Implementation Details:" and "Key Change:" sections as they are repetitious.