Insertion Anchoring

[sageserpent-open]

This follows on from the comment in issue #29 - Edit Anchoring.

This is to handle propagation of inserted text that is brand new and therefore neither an edit nor the destination of a move.

The expectation is for such inserted text to follow the adjacent sections or section as a propagated insertion.

If there are two adjacent sections and they do not move in parallel, then this is a kind of divergence.

[sageserpent-open]

This depends on #38 as it needs to know what sections have moved.

[sageserpent-open]

TODO: once this is deemed complete, do a manual test using the example discussed in #39 and #40.

As an aide-memoire, checkout branch splitPostprocessingOfMergeResult in Kinetic Merge's repository and merge in tag issue-39-dodgy-revision using some variation of the command line:

.../kinetic-merge --no-commit --match-threshold=03  -J-Dlogback-root-level=DEBUG  issue-39-dodgy-revision

[sageserpent-open]

Given the deletion of a file on one side with the removal of all of its content (leaving an empty file) on the other side, Git merges this as a simple conflict between outright deletion and leaving the empty file in place.

Inspection of the code in Kinetic Merge seems to indicate this will also be the case, let's write a quick test to confirm...

[sageserpent-open]

New test added in: 31725230153edb95af1597104ffb18d19e237db0, this passed immediately, as expected; it also detected two injected faults.

[sageserpent-open]

NOTE: while working on this, noticed the check for collisions of multiple edits / deletions being propagated.

That check will flag multiple edit sections as being an error - this is fine in itself, except that if all the sections refer to the same content (albeit at different locations), then this will also be treated as an error. That seems to be too stringent. If all the edit locations feature the same edit, they may as well propagate that shared edit to the their new destinations.

A similar check will be required when migrating insertions from multiple locations to the same destination.

[sageserpent-open]

Getting close to the finish line with: 3d5d14703cf6d8089dc4476b642e39109c462f7b.

CodeMotionAnalysisExtensionTest now passes all of its tests, including the migration of insertions.

However, MainTest has an outstanding test failure in anEditAndADeletionPropagatingThroughAFileCondensation. The other two tests that were previously failing due to lack of insertion migration are passing.

The code is a bit of a mess too - need to look into this business of whether to consider sections of identical content as being one thing (this seems right) for both insertions and for plain old edits.

The diagnostic for colliding insertions needs to be written - right now there is just a placeholder.

The same kind of output that tracks propagated edits / deletions should be furnished for migrated insertions.

I should probably use the same terminology for both edits / deletion and for insertions - choose either 'propagated' or 'migrated' and stick with it. They may have different bits of implementation, but they are doing the same thing from the user's point of view.

[sageserpent-open]

Let's look at the test failure in MainTest.anEditAndADeletionPropagatingThroughAFileCondensation ...

The test starts with two files and condenses them into one - for this particular failure both of the original files disappear on one of the branches; we expect code motion to move edits in on the other branch into the new condensed file.

The original two files:

pathPrefix1/CasesLimitStrategy.java (Interface only) pathPrefix1/CasesLimitStrategies.java (Module-style class with factory methods containing anonymous interface implementations)

The replacement condensed file:

pathPrefix1/pathPrefix2/CasesLimitStrategy.java

The two original files are edited separately - method moreCasesToDo is renamed to moreToDo, method whatIsThisFor is added in the interface and the anonymous classes. Some comments have two lines fused back into one with the rename changes, losing an asterisk in the process.

[sageserpent-open]

Expected content of condensed file pathPrefix1/pathPrefix2/CasesLimitStrategy.java after the merge:

package com.sageserpent.americium.java;

import com.google.common.base.Preconditions;

import java.time.Duration;
import java.time.Instant;
import java.util.function.Consumer;
import java.util.function.Function;
import java.util.function.Predicate;

import static scala.jdk.javaapi.DurationConverters.toJava;

/**
 * Strategy used to limit the emission of cases by the implementation of
 * {@link Trials}. These are supplied by client code when calling
 * {@link Trials#withStrategy(Function)}.
 *
 * @apiNote Instances are expected to be stateful, so they should not be
 * reused when calling the aforementioned overloads.
 */
public interface CasesLimitStrategy {
    /**
     * Limits test case emission using a time budget that starts when the
     * strategy is first consulted via {@link CasesLimitStrategy#moreToDo()}.
     *
     * @param timeBudget How long to allow a testing cycle to continue to
     *                   emit cases.
     * @return A fresh strategy instance - the time budget is not consumed
     * until the first call to {@link CasesLimitStrategy#moreToDo()}.
     */
    static CasesLimitStrategy timed(final Duration timeBudget) {
        return new CasesLimitStrategy() {
            Instant deadline = Instant.MAX;

            @Override
            public boolean moreToDo() {
                if (deadline.equals(Instant.MAX)) {
                    deadline = Instant.now().plus(timeBudget);
                }

                return !Instant.now().isAfter(deadline);
            }

            @Override
            public void noteRejectionOfCase() {

            }

            @Override
            public void noteEmissionOfCase() {

            }

            @Override
            public void noteStarvation() {

            }

            @Override
            public boolean legacyMethod(int whatIsThisFor){ return true; }
        };
    }

    /**
     * Limits test case emission using a time budget that starts when the
     * strategy is first consulted via {@link CasesLimitStrategy#moreToDo()}.
     *
     * @param timeBudget How long to allow a testing cycle to continue to
     *                   emit cases.
     * @return A fresh strategy instance - the time budget is not consumed
     * until the first call to {@link CasesLimitStrategy#moreToDo()}.
     */
    static CasesLimitStrategy timed(
            final scala.concurrent.duration.FiniteDuration timeBudget) {
        return timed(toJava(timeBudget));
    }

    /**
     * Emulation of Scalacheck's approach to limiting emission of test cases.
     *
     * @param maximumNumberOfCases   *Upper* limit on the number of cases
     *                               emitted. <b>For Scalacheck aficionados:
     *                               the name reflects the fact that this is
     *                               a limit, contrast with Scalacheck's
     *                               {@code minSuccessfulTests}.</b>
     * @param maximumStarvationRatio Maximum ratio of case starvation versus
     *                               case emission.
     * @return A fresh strategy instance.
     * @implNote Like Scalacheck, the strategy will allow {@code
     * maximumNumberOfCases * maximumStarvationRatio} starvation to take
     * place before giving up.
     */
    static CasesLimitStrategy counted(int maximumNumberOfCases,
                                      double maximumStarvationRatio) {
        return new CasesLimitStrategy() {
            int numberOfCasesEmitted = 0;
            int starvationCount = 0;

            {
                Preconditions.checkArgument(0 <= maximumNumberOfCases);
                Preconditions.checkArgument(0 <= maximumStarvationRatio);
            }

            @Override
            public boolean moreToDo() {
                return maximumNumberOfCases > numberOfCasesEmitted &&
                       starvationCount <=
                       maximumNumberOfCases * maximumStarvationRatio;
            }

            @Override
            public void noteRejectionOfCase() {
                numberOfCasesEmitted -= 1;
                starvationCount += 1;
            }

            @Override
            public void noteEmissionOfCase() {
                numberOfCasesEmitted += 1;
            }

            @Override
            public void noteStarvation() {
                starvationCount += 1;
            }

            @Override
            public boolean legacyMethod(int whatIsThisFor){ return true; }
        };
    }

    /**
     * Query used by the implementation of {@link Trials} to control the
     * emission of new cases.
     *
     * @return True to signal that more cases should be emitted, false to
     * stop emission.
     * @apiNote Once a call returns false, there should be no further
     * interaction with the strategy by the implementation of {@link Trials}
     * except for additional calls to this method.
     */
    boolean moreToDo();

    /**
     * Notes that inlined case filtration in a test body has rejected a case.
     *
     * @apiNote This is <b>not</b> called when the filtration provided by
     * {@link Trials#filter(Predicate)} rejects a case. When this method is
     * called, there should have been a corresponding call to
     * {@link CasesLimitStrategy#noteEmissionOfCase} concerning the same
     * implied test case that is being backed out of by this method's call.
     */
    void noteRejectionOfCase();

    /**
     * Notes that a case has been successfully emitted. The case is
     * guaranteed to have been constructed in a different way from all others
     * emitted within a call to
     * {@link Trials.SupplyToSyntax#supplyTo(Consumer)}.
     *
     * @apiNote Although each emitted case has been uniquely constructed,
     * this does not mean that it is definitely unique in terms of equality;
     * for one thing, the equality may be unable to distinguish between
     * instances constructed in different ways and for another, the rendition
     * of a test case may flatten information causing collisions between test
     * cases built in different ways.
     */
    void noteEmissionOfCase();

    /**
     * Notes that a case has not been successfully emitted. This can be due
     * to it being a duplicate of an earlier case emitted previously in a
     * call to {@link Trials.SupplyToSyntax#supplyTo(Consumer)}, or may be
     * due to the filtration provided by {@link Trials#filter(Predicate)}
     * rejecting a case, or may be due to the complexity limit being breached.
     *
     * @apiNote This is  <b>not</b> called due to inlined test filtration -
     * that is handled by {@link CasesLimitStrategy#noteRejectionOfCase}.
     */
    void noteStarvation();

    boolean legacyMethod(int whatIsThisFor);
}

[sageserpent-open]

Actual content of condensed file pathPrefix1/pathPrefix2/CasesLimitStrategy.java after the merge:

package com.sageserpent.americium.java;

import com.google.common.base.Preconditions;

import java.time.Duration;
import java.time.Instant;
import java.util.function.Consumer;
import java.util.function.Function;
import java.util.function.Predicate;

import static scala.jdk.javaapi.DurationConverters.toJava;

/**
 * Strategy used to limit the emission of cases by the implementation of
 * {@link Trials}. These are supplied by client code when calling
 * {@link Trials#withStrategy(Function)}.
 *
 * @apiNote Instances are expected to be stateful, so they should not be
 * reused when calling the aforementioned overloads.
 */
public interface CasesLimitStrategy {
    /**
     * Limits test case emission using a time budget that starts when the
     * strategy is first consulted via
     {@link CasesLimitStrategy#moreToDo()}.
     *
     * @param timeBudget How long to allow a testing cycle to continue to
     *                   emit cases.
     * @return A fresh strategy instance - the time budget is not consumed
     * until the first call to {@link CasesLimitStrategy#moreToDo()}.
     */
    static CasesLimitStrategy timed(final Duration timeBudget) {
        return new CasesLimitStrategy() {
            Instant deadline = Instant.MAX;

            @Override
            public boolean moreToDo() {
                if (deadline.equals(Instant.MAX)) {
                    deadline = Instant.now().plus(timeBudget);
                }

                return !Instant.now().isAfter(deadline);
            }

            @Override
            public void noteRejectionOfCase() {

            }

            @Override
            public void noteEmissionOfCase() {

            }

            @Override
            public void noteStarvation() {

            }
        legacyMethod(int whatIsThisFor){ return true; }
        };
    }

    /**
     * Limits test case emission using a time budget that starts when the
     * strategy is first consulted via
     {@link CasesLimitStrategy#moreToDo()}.
     *
     * @param timeBudget How long to allow a testing cycle to continue to
     *                   emit cases.
     * @return A fresh strategy instance - the time budget is not consumed
     * until the first call to {@link CasesLimitStrategy#moreToDo()}.
     */
    static CasesLimitStrategy timed(
            final scala.concurrent.duration.FiniteDuration timeBudget) {
        return timed(toJava(timeBudget));
    }

    /**
     * Emulation of Scalacheck's approach to limiting emission of test cases.
     *
     * @param maximumNumberOfCases   *Upper* limit on the number of cases
     *                               emitted. <b>For Scalacheck aficionados:
     *                               the name reflects the fact that this is
     *                               a limit, contrast with Scalacheck's
     *                               {@code minSuccessfulTests}.</b>
     * @param maximumStarvationRatio Maximum ratio of case starvation versus
     *                               case emission.
     * @return A fresh strategy instance.
     * @implNote Like Scalacheck, the strategy will allow {@code
     * maximumNumberOfCases * maximumStarvationRatio} starvation to take
     * place before giving up.
     */
    static CasesLimitStrategy counted(int maximumNumberOfCases,
                                      double maximumStarvationRatio) {
        return new CasesLimitStrategy() {
            int numberOfCasesEmitted = 0;
            int starvationCount = 0;

            {
                Preconditions.checkArgument(0 <= maximumNumberOfCases);
                Preconditions.checkArgument(0 <= maximumStarvationRatio);
            }

            @Override
            public boolean moreToDo() {
                return maximumNumberOfCases > numberOfCasesEmitted &&
                       starvationCount <=
                       maximumNumberOfCases * maximumStarvationRatio;
            }

            @Override
            public void noteRejectionOfCase() {
                numberOfCasesEmitted -= 1;
                starvationCount += 1;
            }

            @Override
            public void noteEmissionOfCase() {
                numberOfCasesEmitted += 1;
            }

            @Override
            public void noteStarvation() {
                starvationCount += 1;
            }
        };
    }

    /**
     * Query used by the implementation of {@link Trials} to control the
     * emission of new cases.
     *
     * @return True to signal that more cases should be emitted, false to
     * stop emission.
     * @apiNote Once a call returns false, there should be no further
     * interaction with the strategy by the implementation of {@link Trials}
     * except for additional calls to this method.
     */
    boolean moreToDo();

    /**
     * Notes that inlined case filtration in a test body has rejected a case.
     *
     * @apiNote This is <b>not</b> called when the filtration provided by
     * {@link Trials#filter(Predicate)} rejects a case. When this method is
     * called, there should have been a corresponding call to
     * {@link CasesLimitStrategy#noteEmissionOfCase} concerning the same
     * implied test case that is being backed out of by this method's call.
     */
    void noteRejectionOfCase();

    /**
     * Notes that a case has been successfully emitted. The case is
     * guaranteed to have been constructed in a different way from all others
     * emitted within a call to
     * {@link Trials.SupplyToSyntax#supplyTo(Consumer)}.
     *
     * @apiNote Although each emitted case has been uniquely constructed,
     * this does not mean that it is definitely unique in terms of equality;
     * for one thing, the equality may be unable to distinguish between
     * instances constructed in different ways and for another, the rendition
     * of a test case may flatten information causing collisions between test
     * cases built in different ways.
     */
    void noteEmissionOfCase();

    /**
     * Notes that a case has not been successfully emitted. This can be due
     * to it being a duplicate of an earlier case emitted previously in a
     * call to {@link Trials.SupplyToSyntax#supplyTo(Consumer)}, or may be
     * due to the filtration provided by {@link Trials#filter(Predicate)}
     * rejecting a case, or may be due to the complexity limit being breached.
     *
     * @apiNote This is  <b>not</b> called due to inlined test filtration -
     * that is handled by {@link CasesLimitStrategy#noteRejectionOfCase}.
     */
    void noteStarvation();
boolean legacyMethod(int whatIsThisFor);
}

[sageserpent-open]

Leftover in original file pathPrefix1/CasesLimitStrategy.java after the merge (should have been deleted, but is reported as a deletion versus modification conflict):

@Override
            public boolean @Override
            public boolean legacyMethod(int whatIsThisFor){ return true; }
        };
    }

}

[sageserpent-open]

NOTE: sticking with confusing 'propagation' versus 'migration' terminology for now...

Without actually debugging, it seems that the comment changes weren't completely propagated to the condensed file. The rename did propagate, but the fusion of the two lines wasn't, although the loss of the leading comment asterisk was!

Another gotcha is that while the interface definition of method whatIsThisFor was inserted cleanly (OK, the whitespace is weird, but that's to be expected with code motion), the method definition in the first anonymous class lost its @Override annotation and its public modifier.

Worse still, the method definition in the second anonymous class was lost completely.

The lost pieces are left stranded in the original file, along with some miscellaneous closing braces and semicolons that I'm not sure about.

[sageserpent-open]

Regarding the comment line fusion not being propagated, this is down to a correct merge of CasesLimitStrategies.java - deleted on the side with the condensation, edited on the other side. That merge interprets the section:

    /**
     *  ... strategy is first consulted via

(Has a trailing newline) as being deleted on the left - so far, so good. This matches the minor edit of that section to lose the whitespace on the editing branch:

    /**
     *  ... strategy is first consulted via 

(Now has a trailing space character).

Because the editing branch is on the right, the dominance rules ignore it, so it is the left-hand's version - the one being deleted in CasesLimitStrategy.java and that has moved to a new location in the condensed file - that is left as is as a brand new section in its new location.

So the line-fusion edit is lost!

[sageserpent-open]

Let's park that line-fusion issue for a bit - possibly one for another ticket.

[sageserpent-open]

Digging through the logs revealed that the presence of ambiguous matches for the snippet

@Override
public boolean

blocked the anchoring, and also forced what was considered to be inserted code to be shrunk down.

Increasing the minimum ambiguous match size to 5 sorted that out; the insertions now migrate correctly, although the result still has the line-fusion problem discussed above, and the indentation looks hokey.

(The previous leftovers in pathPrefix1/CasesLimitStrategy.java have vanished, so that file is now deleted by the merge, which is good).

This wouldn't be acceptable for Python, or Scala 3 / F# / OCaml, but for a curly-brace language it's fine, especially if an auto-formatter is applied to the result of the merge.

The test in question still fails as of ee69833bdc147f45e0bd15c4309b9f0f4eed0d41, but we're close...

[sageserpent-open]

Examining the differences between the actual and the expected merge contents of pathPrefix1/pathPrefix2/CasesLimitStrategy.java:

Not too bad!

(UPDATE: just noticed the addtional premature closing brace between the factory methods and the interface methods proper. Oops.)

[sageserpent-open]

Disabled the failing test in 06c7b1fb4d10b4dd3055d0cdbfd17efaf600a22a, this is waiting on #9.SEE NEXT COMMENT

Now to do all the tidying up, addressing section equality versus section content equality and getting decent error diagnostics and logging / output for the user...

[sageserpent-open]

The results of the merge in MainTest.anEditAndADeletionPropagatingThroughAFileCondensation:

package com.sageserpent.americium.java;

import com.google.common.base.Preconditions;

import java.time.Duration;
import java.time.Instant;
import java.util.function.Consumer;
import java.util.function.Function;
import java.util.function.Predicate;

import static scala.jdk.javaapi.DurationConverters.toJava;

/**
 * Strategy used to limit the emission of cases by the implementation of
 * {@link Trials}. These are supplied by client code when calling
 * {@link Trials#withStrategy(Function)}.
 *
 * @apiNote Instances are expected to be stateful, so they should not be
 * reused when calling the aforementioned overloads.
 */
public interface CasesLimitStrategy {
    /**
     * Limits test case emission using a time budget that starts when the
     * strategy is first consulted via
     {@link CasesLimitStrategy#moreToDo()}.
     *
     * @param timeBudget How long to allow a testing cycle to continue to
     *                   emit cases.
     * @return A fresh strategy instance - the time budget is not consumed
     * until the first call to {@link CasesLimitStrategy#moreToDo()}.
     */
    static CasesLimitStrategy timed(final Duration timeBudget) {
        return new CasesLimitStrategy() {
            Instant deadline = Instant.MAX;

            @Override
            public boolean moreToDo() {
                if (deadline.equals(Instant.MAX)) {
                    deadline = Instant.now().plus(timeBudget);
                }

                return !Instant.now().isAfter(deadline);
            }

            @Override
            public void noteRejectionOfCase() {

            }

            @Override
            public void noteEmissionOfCase() {

            }

            @Override
            public void noteStarvation() {

            }
            @Override
            public boolean legacyMethod(int whatIsThisFor){ return true; }
        };
    }

    /**
     * Limits test case emission using a time budget that starts when the
     * strategy is first consulted via
     {@link CasesLimitStrategy#moreToDo()}.
     *
     * @param timeBudget How long to allow a testing cycle to continue to
     *                   emit cases.
     * @return A fresh strategy instance - the time budget is not consumed
     * until the first call to {@link CasesLimitStrategy#moreToDo()}.
     */
    static CasesLimitStrategy timed(
            final scala.concurrent.duration.FiniteDuration timeBudget) {
        return timed(toJava(timeBudget));
    }

    /**
     * Emulation of Scalacheck's approach to limiting emission of test cases.
     *
     * @param maximumNumberOfCases   *Upper* limit on the number of cases
     *                               emitted. <b>For Scalacheck aficionados:
     *                               the name reflects the fact that this is
     *                               a limit, contrast with Scalacheck's
     *                               {@code minSuccessfulTests}.</b>
     * @param maximumStarvationRatio Maximum ratio of case starvation versus
     *                               case emission.
     * @return A fresh strategy instance.
     * @implNote Like Scalacheck, the strategy will allow {@code
     * maximumNumberOfCases * maximumStarvationRatio} starvation to take
     * place before giving up.
     */
    static CasesLimitStrategy counted(int maximumNumberOfCases,
                                      double maximumStarvationRatio) {
        return new CasesLimitStrategy() {
            int numberOfCasesEmitted = 0;
            int starvationCount = 0;

            {
                Preconditions.checkArgument(0 <= maximumNumberOfCases);
                Preconditions.checkArgument(0 <= maximumStarvationRatio);
            }

            @Override
            public boolean moreToDo() {
                return maximumNumberOfCases > numberOfCasesEmitted &&
                        starvationCount <=
                                maximumNumberOfCases * maximumStarvationRatio;
            }

            @Override
            public void noteRejectionOfCase() {
                numberOfCasesEmitted -= 1;
                starvationCount += 1;
            }

            @Override
            public void noteEmissionOfCase() {
                numberOfCasesEmitted += 1;
            }

            @Override
            public void noteStarvation() {
                starvationCount += 1;
            }
            @Override
            public boolean legacyMethod(int whatIsThisFor){ return true; }
        };
    }

}
/**
 * Query used by the implementation of {@link Trials} to control the
 * emission of new cases.
 *
 * @return True to signal that more cases should be emitted, false to
 * stop emission.
 * @apiNote Once a call returns false, there should be no further
 * interaction with the strategy by the implementation of {@link Trials}
 * except for additional calls to this method.
 */
boolean moreToDo();

/**
 * Notes that inlined case filtration in a test body has rejected a case.
 *
 * @apiNote This is <b>not</b> called when the filtration provided by
 * {@link Trials#filter(Predicate)} rejects a case. When this method is
 * called, there should have been a corresponding call to
 * {@link CasesLimitStrategy#noteEmissionOfCase} concerning the same
 * implied test case that is being backed out of by this method's call.
 */
void noteRejectionOfCase();

/**
 * Notes that a case has been successfully emitted. The case is
 * guaranteed to have been constructed in a different way from all others
 * emitted within a call to
 * {@link Trials.SupplyToSyntax#supplyTo(Consumer)}.
 *
 * @apiNote Although each emitted case has been uniquely constructed,
 * this does not mean that it is definitely unique in terms of equality;
 * for one thing, the equality may be unable to distinguish between
 * instances constructed in different ways and for another, the rendition
 * of a test case may flatten information causing collisions between test
 * cases built in different ways.
 */
void noteEmissionOfCase();

/**
 * Notes that a case has not been successfully emitted. This can be due
 * to it being a duplicate of an earlier case emitted previously in a
 * call to {@link Trials.SupplyToSyntax#supplyTo(Consumer)}, or may be
 * due to the filtration provided by {@link Trials#filter(Predicate)}
 * rejecting a case, or may be due to the complexity limit being breached.
 *
 * @apiNote This is  <b>not</b> called due to inlined test filtration -
 * that is handled by {@link CasesLimitStrategy#noteRejectionOfCase}.
 */
void noteStarvation();
boolean legacyMethod(int whatIsThisFor);
}

The interface declaration and the two implementations of legacyMethod have migrated.

I've just noticed the closing } that comes before the interface method definitions. (It can be seen in the diff above, too).

Need to address that first, as that is clearly wrong from the compilation correctness point of view...

[sageserpent-open]

Investigated the closing brace issue, raised this ticket #42 for it as it not specifically part of this work, and have tweaked the failing test in d5df1eb361b6c9a1e90fbe85d41142780e17274a. It passes now, even with the dubious line-fusion still waiting on #9.

Huzzah!

[sageserpent-open]

Commit d5df1eb361b6c9a1e90fbe85d41142780e17274a served as the base for #42 , which has been merged to branch main.

[sageserpent-open]

Reopening - have to do the tidying work...

[sageserpent-open]

Final tidying up done in Git commit SHA: 362d0bc8788bee268ddc91db3d0af04dc42928e9.

[sageserpent-open]

This went out in release 1.2.0, Git commit SHA: 4d1935beef2b5923417904895884de8c2e749c6e.