Tests for transaction validation

[nickchapman-da]

Testing for transaction validation (isReplayedBy)

• Tweak based testing
• methodically tweak every field of every kind of GenActionNode
• classifying each tweak as SIGNIFICANT or INSIGNIFICANT
• test over a range of hand constructed sample transactions (no scalacheck here!)

This PR advances #6665

[nickchapman-da]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[remyhaemmerle-da]

I like it very much. Thanks a lot.

[remyhaemmerle-da]

just curious to know

• why it is a val ?
• why it is not private like the other (I do not really care of private for tests) ?

[nickchapman-da]

val? -- no good reason. I changed to def.
not private? -- oversight. fixed now.
(good spot, thanks)
the reason I liked to use private here (when I remembered!) is that is ensures that everything definition has a use.

[remyhaemmerle-da]

Why this one is not a val ?

[nickchapman-da]

not a val? -- no good reason. changed to a val.

If you want to know the actual reason... during development I often use ??? for code I haven''t yet written, but I often want to check the overall structure compiles. However, scalac complains about unreachable code for combinations of val/???, so I must use def as a temporary workaround.

[remyhaemmerle-da]

Just an alternative, not really better.

Suggested change

	case Some(x) => None :: List(samKWM1, samKWM2, samKWM3).filter(y => x != y).map(Some(_))
	case Some(x) => None :: List(samKWM1, samKWM2, samKWM3).collect{ case y if x != y => Some(y) }```

[remyhaemmerle-da]

I am perfectly fine with the current state of the Tweak object, but have you consider using PartialFunction ? I find it fits particularly well your use case.

See the following suggestion together with the two suggestions I made line 259 and line 276 (and two others)

Suggested change

	private object Tweak {
	def single[X](f1: X => Option[X]): Tweak[X] = {
	def f(x: X) = f1(x) match {
	case None => Nil
	case Some(y) => List(y)
	}
	new Tweak(f)
	}
	def apply[X](f: X => List[X]): Tweak[X] = {
	new Tweak(f)
	}
	}
	private object Tweak {
	def single[X](f1: PartialFunction[X, X]): Tweak[X] =
	Tweak(f1.andThen(List(_)))
	def apply[X](f: PartialFunction[X, List[X]]): Tweak[X] =
	new Tweak(f.applyOrElse(List.empty))
	}

[cocreature]

partial function to list seems a bit odd. Afaict we have no difference between returning an empty list and the function not being defined.

[remyhaemmerle-da]

new Tweak(f.orElse(_ => List.empty[X])) may work better than new Tweak(f.applyOrElse(List.empty))

[nickchapman-da]

Thank you for the suggestion about PartialFunction. I am aware of it, but to be honest it's not something that is in my everyday scala toolbox. Also, I would be a bit unhappy to loose exhaustiveness checking for pattern matches. However, perhaps it would be idiomatic for the use case here. I wont make the change in this PR. Let's discuss offline & you can try to persuade me. :)

[S11001001]

I don't see any use for the Tweak class. A type synonym will serve equally well.

When you write a function with partial function syntax, and the expected type is a non-partial function, you will still get exhaustiveness checking. Broadly speaking, this violates the Liskov Substitution Principle (because an expression that satisfies type t does not satisfy type u >: t if warnings are errors), but not in a way that violates soundness.

So that means you can write

private type Tweak[T] = T => List[T]
private object Tweak {
  def apply[T](f: Tweak[T]): Tweak[T] = f
}

Tweak[Option[Location]] {
  case None => //blah blah
  // I get an exhaustiveness warning if I don't handle Some
}

[remyhaemmerle-da]

With the PartialFunction change made above:

Suggested change

	private val tweakCreateCoid = Tweak.single[Node] {
	case nc: Node.NodeCreate[_] => Some(nc.copy(coid = changeContractId(nc.coid)))
	case _ => None
	}
	private val tweakCreateCoid = Tweak.single[Node] {
	case nc: Node.NodeCreate[_] => nc.copy(coid = changeContractId(nc.coid))
	}

[S11001001]

It is perfectly reasonable to have another method in object Tweak that takes a PartialFunction instead and adds the Seq.empty case with orElse, producing a total function. You are getting no exhaustiveness-checking value out of many of these tweak definitions, and defining them as @remyhaemmerle-da suggests would clean them up.

[remyhaemmerle-da]

Again not a request for change.

You could consider using scala check TableDrivenPropertyChecks (See for example SBuiltinTest)
This as the following advantages:

• it runs all the test cases (and not stop at the first failing one)
• it reports slightly nicer errors
  @S11001001 are there other advantages ?

[nickchapman-da]

I'll have a look at this...

[S11001001]

forEvery is already doing a lot of the stuff for you so you don't have to do it yourself. For example, a nested forEvery here would simply report the txA and txB in a nice way as part of the failure message, so you would not have to jam it into the fail string.

And that makes it so that you can simply use shouldBe or inside without using fail at all, and still getting all the stuff you might need to know to understand failures reported.

[remyhaemmerle-da]

Why not simply res shouldBe Right(())

[nickchapman-da]

because it is quite helpful to be able to see the txA and txB

[remyhaemmerle-da]

👍

[S11001001]

forEvery solves this problem nicely, as mentioned in #10167 (comment).

But there's another aspect here that really illustrates why it's useful to track down the existing combinators rather than use scala-library to simulate them. There are two kinds of failures of these kinds of tests: break-everything, and break-something. As written, these tests stop on the first failure. So you don't really know which of those situations you're in.

With forEvery, you can immediately tell from one test run whether you've broken everything, just a few things, or a single edge case. It's a bit like the difference between having separate test declarations for each test, and just putting every assertion in the whole file into a single test case.

Redefining this functionality is annoying. And it is quite useful; #8879 was more or less an exercise of adding cases, tweaking or rewriting part of the algorithm, and quickly getting feedback on every shortcoming of whatever change I had just made--whether there was just one detail I had forgotten, or everything collapsed and I had hit a dead end.

[remyhaemmerle-da]

I would better not use println.
If you want to display a message on failure, better use fail.
See ScalaTest assertions

[nickchapman-da]

ok, I used fail

[remyhaemmerle-da]

With the PartialFunction change for Tweak.apply

Suggested change

	private val tweakExerciseExerciseResultSome = Tweak[Node] { //sig
	case ne: Node.NodeExercises[_, _] =>
	ne.exerciseResult match {
	case None => Nil
	case Some(v) =>
	List(
	ne.copy(exerciseResult = Some(changeValue(v))),
	ne.copy(exerciseResult = None),
	)
	}
	case _ => Nil
	}
	private val tweakExerciseExerciseResultSome = Tweak[Node] { //sig
	case ne: Node.NodeExercises[_, _] if ne.exerciseResult.isDefined =>
	List(
	ne.copy(exerciseResult = ne.exerciseResult.map(changeValue)),
	ne.copy(exerciseResult = None),
	)
	}
	}

[remyhaemmerle-da]

with the PartialFunction change in Tweak.single.

Suggested change

	private def tweakExerciseByKey(whenVersion: TransactionVersion => Boolean) = Tweak.single[Node] {
	case ne: Node.NodeExercises[_, _] =>
	if (whenVersion(ne.version)) Some(ne.copy(byKey = changeBoolean(ne.byKey)))
	else None
	case _ => None
	}
	private def tweakExerciseByKey(whenVersion: TransactionVersion => Boolean) = Tweak.single[Node] {
	case ne: Node.NodeExercises[_, _] if whenVersion(ne.version) =>
	ne.copy(byKey = changeBoolean(ne.byKey))
	}

[cocreature]

Nice, I think there is a set of tests that is missing here that we should add but doesn’t have to be in this PR and some of this might also become redundant by adding the normalization:

Anything around items having different numbers of children: e.g., different number of children in an exercise rollback but also different number of record fields. More generally, I think there are probably a few more tests around values (e.g., tweak of record constructor) that could be useful.

[cocreature]

partial function to list seems a bit odd. Afaict we have no difference between returning an empty list and the function not being defined.

[nickchapman-da]

partial function to list seems a bit odd. Afaict we have no difference between returning an empty list and the function not being defined.

A Tweak[X] is a total function from X to List[X]

(1) A non-empty return list provides a list of value-replacements which we want to apply here.
(2) An empty list means, this tweak does not apply here.

There is no concept of a tweak being applicable, but with a empty list of value-replacements. Or at least the meaning of this is indistinguishable from (2).

[remyhaemmerle-da]

partial function to list seems a bit odd. Afaict we have no difference between returning an empty list and the function not being defined.

A Tweak[X] is a total function from X to List[X]

I think Mortiz is referring to my suggestion to use PartialTransaction.
I do not propose to change Tweak, but the two builders to use PartialTransaction instead of total function, specially because You use partial function notation in all your usage of the builders.

Here is a concrete example of what I have in mind.

To answer @cocreature concern I do not find to have a "partial function to list" so odd, the idea is just to understand that where the function passed to the builder is undefined, we just use the default value List.empty.
I think this make the writing of test case much less tedious.

[S11001001]

I don't see any use for the Tweak class. A type synonym will serve equally well.

When you write a function with partial function syntax, and the expected type is a non-partial function, you will still get exhaustiveness checking. Broadly speaking, this violates the Liskov Substitution Principle (because an expression that satisfies type t does not satisfy type u >: t if warnings are errors), but not in a way that violates soundness.

So that means you can write

private type Tweak[T] = T => List[T]
private object Tweak {
  def apply[T](f: Tweak[T]): Tweak[T] = f
}

Tweak[Option[Location]] {
  case None => //blah blah
  // I get an exhaustiveness warning if I don't handle Some
}

[S11001001]

This kind of code should broadly use Seq instead of List. It would probably be profitably idiomatic to replace every List with Seq here.

The only legitimate reasons to use List directly are

1. you need to traverse or
2. need a functor/monad instance, or
3. you rely specifically on the structure of a list, i.e. cheap push/pop

[S11001001]

It is perfectly reasonable to have another method in object Tweak that takes a PartialFunction instead and adds the Seq.empty case with orElse, producing a total function. You are getting no exhaustiveness-checking value out of many of these tweak definitions, and defining them as @remyhaemmerle-da suggests would clean them up.

[S11001001]

All printlns should be removed.

[S11001001]

forEvery is already doing a lot of the stuff for you so you don't have to do it yourself. For example, a nested forEvery here would simply report the txA and txB in a nice way as part of the failure message, so you would not have to jam it into the fail string.

And that makes it so that you can simply use shouldBe or inside without using fail at all, and still getting all the stuff you might need to know to understand failures reported.

[nickchapman-da]

Thanks @stephencompall-DA for your suggestions. Please see: #10214