feat: add ability to run a move during testing #2028

triceo · 2026-01-20T10:17:11Z

This was written entirely using SpecKit. Human+AI collaboration went into the specification; the code produced was 99 % done by Claude Sonnet 4.5.

Important pieces:

The actual code and docs.
spec.md contains the specification of the feature
plan.md (and accompanying data model + API) contains the AI's implementation plan
constitution.md - I drafted some amendments as I was watching AI produce code that didn't look like "our" code.

I'm asking both @Christopher-Chianelli and @zepfred for a very careful code review. I have two reasons for that:

We should all be aware of what is possible with today's tools, what are its good sides and what are its limitations.
We need to pay extra attention to code like this, because it was not produced by any of us.

I do not recommend reviewing commit-by-commit. Review the final product, if you wish to keep your sanity.

triceo · 2026-01-20T11:09:49Z

core/src/main/java/ai/timefold/solver/core/preview/api/move/MoveRunner.java

+     * This class is an internal detail and must not be exposed to the user.
+     */
+    @NullMarked
+    static final class DummyConstraintProvider implements ConstraintProvider {


This forces some changes to Quarkus/Spring.
I made these manually, as I didn't trust the agent to figure it out without all the speccing and planning prerequisites.

Why do you need a DummyConstraintProvider? MoveAsserter works without one.

Copilot

Pull request overview

This pull request adds a new MoveRunner API for testing move implementations during development. The feature was implemented using AI assistance (Claude Sonnet 4.5) based on specifications created through human-AI collaboration.

Changes:

Added new testing API (MoveRunner and MoveRunContext) for executing moves with automatic undo support
Updated Spring Boot and Quarkus auto-configuration to filter out internal test classes
Enhanced configuration utilities to support non-public inner static classes
Added comprehensive test coverage using the new API with builtin moves
Updated documentation with usage examples and best practices

Reviewed changes

Copilot reviewed 25 out of 26 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
MoveRunner.java	New API entry point providing fluent interface for move execution with resource management
MoveRunContext.java	New execution context for permanent and temporary move execution
InnerScoreDirector.java	Added executeTemporarily variant accepting consumer callback
AbstractScoreDirector.java	Implemented new executeTemporarily method delegating to move director
ConfigUtils.java	Enhanced to support non-public inner static class instantiation
TimefoldSolverAutoConfiguration.java	Added UNIQUENESS_PREDICATE to filter internal classes and abstract types
IncludeAbstractClassesEntityScanner.java	Updated to use shared UNIQUENESS_PREDICATE
TimefoldProcessor.java	Added UNIQUENESS_PREDICATE and renamed logger constant
Moves.java	Updated swap methods with proper type parameterization
neighborhoods.adoc	Added comprehensive MoveRunner API documentation
*MoveTest.java (6 files)	New test classes demonstrating MoveRunner usage with builtin moves
MoveRunnerTest.java	Unit tests for MoveRunner API
Specification files	Complete feature specification, design, and implementation plan

...uarkus/deployment/src/main/java/ai/timefold/solver/quarkus/deployment/TimefoldProcessor.java

sonarqubecloud · 2026-01-20T12:01:33Z

Quality Gate passed

Issues
2 New issues
0 Accepted issues

Measures
0 Security Hotspots
97.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

Christopher-Chianelli

This seems to be duplicating MoveAsserter (which also has the benefit of not needing a dummy ConstraintProvider to function). The tests relies on the initial values of entities in generated solutions; I would rather the entities values be set explictly when reading tests.

Christopher-Chianelli · 2026-01-20T14:36:24Z

.specify/specs/001-move-running-api/contracts/api-contract.md

+    try (var runner = MoveRunner.build(Solution.class, Entity.class)) {
+        // Execute multiple moves in sequence
+        var context1 = runner.using(solution1);
+        context1.execute(move1);


Where are move1/move2 defined? (I know this is for a docs and doesn't need to compile per se, but the other examples define their moves)

Christopher-Chianelli · 2026-01-20T14:55:29Z

.specify/specs/001-move-running-api/quickstart.md

+var solution = createTestSolution();
+try {
+    try (var runner = MoveRunner.build(...)) {
+        runner.using(solution).executeTemporarily(move, assertions);
+    }
+} catch (Exception e) {
+    // Solution is now in undefined state - discard it
+    solution = createTestSolution(); // Create fresh instance
+}


If a test looked like this, I would call it a bad unit test.

Christopher-Chianelli · 2026-01-20T15:51:13Z

core/src/main/java/ai/timefold/solver/core/config/util/ConfigUtils.java

        try {
-            return clazz.getDeclaredConstructor().newInstance();
+            var ctor = clazz.getDeclaredConstructor();
+            ctor.setAccessible(true); // For inner static non-public classes; public noarg ctors are effectively not.


This will fail when Java enforces encapsulation; when is setAccessible needed?

Christopher-Chianelli · 2026-01-20T15:53:52Z

core/src/main/java/ai/timefold/solver/core/preview/api/move/builtin/Moves.java

+    @SuppressWarnings("unchecked")
+    public static <Solution_, Entity_, Value_> Move<Solution_> swap(
+            PlanningVariableMetaModel<Solution_, Entity_, Value_> variableMetaModel, Entity_ leftEntity, Entity_ rightEntity) {
+        return swap(List.of((PlanningVariableMetaModel<Solution_, Entity_, Object>) variableMetaModel), leftEntity,
+                rightEntity);


The previous implementation was better

Christopher-Chianelli · 2026-01-20T15:58:04Z

core/src/main/java/ai/timefold/solver/core/preview/api/move/MoveRunContext.java

+        Objects.requireNonNull(exceptionHandler, "exceptionHandler");
+
+        try {
+            scoreDirector.executeMove(Objects.requireNonNull(move, "move"));


Should we throw an AssertionError when an exception is not thrown? I would only used this method if I am expecting an exception in a test.

Christopher-Chianelli · 2026-01-20T16:21:27Z

core/src/test/java/ai/timefold/solver/core/preview/api/move/MoveRunnerTest.java

+        var solution = TestdataSolution.generateSolution(2, 2);
+        assertThatThrownBy(() -> runner.using(solution))
+                .isInstanceOf(IllegalStateException.class)
+                .hasMessageContaining("closed");


Probably should be "The MoveRunner cannot be used after it is closed."

Christopher-Chianelli · 2026-01-20T16:24:43Z

docs/src/modules/ROOT/pages/optimization-algorithms/neighborhoods.adoc

+
+[source,java,options="nowrap"]
+----
+try (var runner = MoveRunner.build(SolutionClass.class, EntityClass.class)) {


Suggested change

try (var runner = MoveRunner.build(SolutionClass.class, EntityClass.class)) {

try (var runner = MoveRunner.build(Solution.class, Entity.class)) {

Christopher-Chianelli · 2026-01-20T16:25:16Z

docs/src/modules/ROOT/pages/optimization-algorithms/neighborhoods.adoc

+@Test
+void testChangeMove() {
+    // Arrange
+    var solution = TestdataSolution.generateSolution(2, 2);


I would not use Testdata in the docs and would use an example such as Employee Scheduling

Christopher-Chianelli · 2026-01-20T16:25:58Z

docs/src/modules/ROOT/pages/optimization-algorithms/neighborhoods.adoc

+----
+@Test
+void testMoveThenUndo() {
+    var solution = TestdataSolution.generateSolution(2, 2);


Christopher-Chianelli · 2026-01-20T16:27:02Z

docs/src/modules/ROOT/pages/optimization-algorithms/neighborhoods.adoc

+----
+@Test
+void testMoveWithExceptionHandler() {
+    var solution = createInvalidSolution(); // May cause exceptions


That comment is pointless/not useful; I would create a move that uses an out of bound index

zepfred · 2026-01-20T17:49:03Z

core/src/main/java/ai/timefold/solver/core/impl/score/director/AbstractScoreDirector.java

    }

+    @Override
+    public void executeTemporarily(Move<Solution_> move, Consumer<SolutionView<Solution_>> consumer) {


First, let's rename the method to executeTemporaryMove for consistency. Additionally, I suggest merging the logic of the new method with the existing one. This way, we can reuse the logic and pass a dummy consumer when it is not necessary.

zepfred · 2026-01-20T17:51:29Z

core/src/main/java/ai/timefold/solver/core/impl/score/director/InnerScoreDirector.java

     */
    InnerScore<Score_> executeTemporaryMove(Move<Solution_> move, boolean assertMoveScoreFromScratch);

+    /**


See my previous related comment. We can provide a default implementation for the method without a consumer.

zepfred · 2026-01-20T17:56:54Z

core/src/main/java/ai/timefold/solver/core/preview/api/move/MoveRunContext.java

+    private final InnerScoreDirector<Solution_, ?> scoreDirector;
+
+    MoveRunContext(InnerScoreDirector<Solution_, ?> scoreDirector) {
+        this.scoreDirector = Objects.requireNonNull(scoreDirector, "scoreDirector");


I wonder how useful it is to check for nullity when using @NullMarked.

zepfred · 2026-01-20T17:59:20Z

core/src/main/java/ai/timefold/solver/core/preview/api/move/MoveRunContext.java

+     * @throws NullPointerException if move or assertions is null
+     * @throws IllegalStateException if the parent MoveRunner has been closed
+     */
+    public void executeTemporarily(Move<Solution_> move, Consumer<SolutionView<Solution_>> assertions) {


Let's rename it to executeTemporaryMove.

zepfred · 2026-01-20T18:06:56Z

core/src/main/java/ai/timefold/solver/core/preview/api/move/MoveRunner.java

+                Objects.requireNonNull(solutionClass, "solutionClass"), entityClasses);
+
+        // Create a Constraint Streams configuration with a dummy constraint provider
+        var scoreDirectorFactoryConfig = new ScoreDirectorFactoryConfig()


I don't like the idea of using a dummy score provider by default. I would prefer to include the score provider as a parameter and move DummyConstraintProvider to a test package. This way, we can reuse the provider across all move test classes without losing cohesion.

We could also provide a facility method that uses the dummy constraint provider by default outside of the MoveRunner class and maintains the current logic of the test move classes.

After reviewing all classes, I see how the dummy score provider simplifies testing. Maybe we can rename the provider class to something like InternalMoveRunnerConstraintProvider and keep the current implementation.

zepfred · 2026-01-20T18:19:00Z

core/src/test/java/ai/timefold/solver/core/preview/api/move/builtin/ChangeMoveTest.java

+    @Test
+    void changeMoveTemporaryMultipleEntities() {
+        // Arrange
+        var solution = TestdataSolution.generateSolution(3, 3);


zepfred

The test coverage looks good. While we have small, self-contained tests, I believe we could move some boilerplate code from these test classes to a parent class to reduce duplication.

The AbstractScoreDirector class can be enhanced, and we should ensure that the names of methods executing temporary moves are consistent with the existing ones.

triceo added 25 commits January 19, 2026 07:30

chore: spec out the feature

cd918ee

Rename the feature

482812b

Move specs dir

d8a4626

We'll go back to spec

d279dcd

Respec

7ef6f60

We have a plan

62e72fa

Respec

06c5ca6

Finish the plan?

e256391

Constitution 1.1

55b3721

Final tasks?

113fbec

Another bump to constitution

3426615

Respec

35f542c

Remove the plan

b38e49d

Final plan?

789b92e

Improve the plan

579c320

Tasks

7a1abc8

More constitution

300bb7d

First phase is done

6765275

Second phase is done, one test failing

3d42546

All tests pass!

97858e7

MoveRunner review

80a99b2

MoveRunContext review

8dcf9e5

Final review

5076acf

Move specs to .specify

e07e152

Docs

f817bca

triceo temporarily deployed to internal January 20, 2026 10:17 — with GitHub Actions Inactive

triceo deployed to documentation (preview) January 20, 2026 10:17 — with GitHub Actions View deployment

triceo requested review from Christopher-Chianelli and zepfred January 20, 2026 10:20

triceo commented Jan 20, 2026

View reviewed changes

Manual fix

f425224

triceo temporarily deployed to internal January 20, 2026 11:11 — with GitHub Actions Inactive

triceo deployed to documentation (preview) January 20, 2026 11:11 — with GitHub Actions View deployment

Remove checklists

287ea1d

triceo temporarily deployed to internal January 20, 2026 11:13 — with GitHub Actions Inactive

triceo deployed to documentation (preview) January 20, 2026 11:13 — with GitHub Actions View deployment

triceo marked this pull request as ready for review January 20, 2026 11:15

triceo requested a review from TomCools as a code owner January 20, 2026 11:15

Copilot AI review requested due to automatic review settings January 20, 2026 11:15

Copilot started reviewing on behalf of triceo January 20, 2026 11:15 View session

Copilot AI reviewed Jan 20, 2026

View reviewed changes

...uarkus/deployment/src/main/java/ai/timefold/solver/quarkus/deployment/TimefoldProcessor.java Show resolved Hide resolved

Sonar

eed37cd

triceo temporarily deployed to internal January 20, 2026 11:43 — with GitHub Actions Inactive

triceo deployed to documentation (preview) January 20, 2026 11:43 — with GitHub Actions View deployment

triceo added the component/neighborhoods label Jan 20, 2026

triceo added this to the v1.31.0 milestone Jan 20, 2026

triceo self-assigned this Jan 20, 2026

Christopher-Chianelli requested changes Jan 20, 2026

View reviewed changes

zepfred reviewed Jan 20, 2026

View reviewed changes

zepfred requested changes Jan 20, 2026

View reviewed changes

	try (var runner = MoveRunner.build(SolutionClass.class, EntityClass.class)) {
	try (var runner = MoveRunner.build(Solution.class, Entity.class)) {

feat: add ability to run a move during testing #2028

Are you sure you want to change the base?

feat: add ability to run a move during testing #2028

Uh oh!

Conversation

triceo commented Jan 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

sonarqubecloud bot commented Jan 20, 2026

Quality Gate passed

Uh oh!

Christopher-Chianelli left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zepfred Jan 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zepfred Jan 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zepfred left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

triceo commented Jan 20, 2026 •

edited

Loading

zepfred Jan 20, 2026 •

edited

Loading

zepfred Jan 20, 2026 •

edited

Loading