[xef-evaluator] - Improvements #568

tomascayuelas · 2023-11-29T11:02:08Z

Improvements:

Content validation
Richer domain
Change DSL
Adding tests

Don't hesitate to comment anything.

javipacheco

🚀 Great job!!

I have tested the evaluator locally and everything works as expected.

evaluator-example/src/main/kotlin/com/xebia/functional/xef/evaluator/examples/TestExample.kt

fedefernandez

This looks great @tomascayuelas. Just one comment, how hard would be to use the kotlin.test framework instead kotest? In Arrow they followed the approach of using kotest just for the assertions due to compatibility reasons with the platform.

tomascayuelas · 2023-11-30T14:11:42Z

This looks great @tomascayuelas. Just one comment, how hard would be to use the kotlin.test framework instead kotest? In Arrow they followed the approach of using kotest just for the assertions due to compatibility reasons with the platform.

Thanks @fedefernandez I'll check it and see if it's simple.

fedefernandez · 2023-11-30T14:21:40Z

@tomascayuelas I made a migration in this commit, in case it helps
2c5a852
Arrow's closed related PRs are also a good source of info

tomascayuelas · 2023-11-30T14:22:50Z

@tomascayuelas I made a migration in this commit, in case it helps

2c5a852

Arrow's closed related PRs are also a good source of info

Ohhh perfect! Thanks @fedefernandez

…lidations.

Adding test for EitherOps functions

fedefernandez

Left some minor comments. I think it would be worthy to address or discuss

fedefernandez · 2023-12-01T10:33:55Z

evaluator/src/main/kotlin/com/xebia/functional/xef/evaluator/EitherOps.kt

+import kotlinx.serialization.encodeToString
+import kotlinx.serialization.json.Json
+
+object EitherOps {


Question, in Kotlin is this a good practice (nested the functions in an object)? I have seen in the project the extension functions defined at top level

No, it's a pretty bad practice sadly. It doesn't play well with auto-complete for imports, in Kotlin.
These should just be top-level function, when we get proper namespaces in Kotlin 2.x this will probably improve but for now they need to be top-level.

evaluator/build.gradle.kts

evaluator/src/main/kotlin/com/xebia/functional/xef/evaluator/models/errors/SuiteSpecError.kt

fedefernandez · 2023-12-01T10:43:23Z

evaluator/src/test/kotlin/com/xebia/functional/xef/evaluator/EitherOpsSpec.kt

+class EitherOpsSpec {
+  private val validSuiteSpec = runBlocking {
+    SuiteSpec(simpleString.next()) {
+      outputDescription { simpleString.next() }


Each time this test is executed, it runs once with random data, right? In that case, I think it would be better to use fixed values because, right now, can lead to flaky tests. Not a big deal though, just a preference.

I see we're using Kotest here, we can potentially piggy back on their SEED which is random and printed on failure.

Otherwise I would at least work with a SEED, and print it somewhere on each build, or use fixed data like @fedefernandez mentioned. I am not a fan of next() at all, since it's quite unpredictable.

fedefernandez · 2023-12-01T10:45:51Z

evaluator/src/test/kotlin/com/xebia/functional/xef/evaluator/ItemSpecBuilderSpec.kt

+  }
+
+  @Test
+  fun shouldInvalidWhenAnyCombinationOfInputAreEmpty() = runTest {


All the invalid tests could be replaced by a property test that generates a tuple of the invalid ItemSpec + the error list.

Should we look into property testing? It's supported by Kotest out-of-the-box, and has some cool stuff like Exhaustive. In contrast to Arb it only defines "edge cases", and makes sure that by minimum the full cartesian product of all edge cases are run by the tests.

That's pretty cool, since it can be used to run the least amount of tests but still cover all edge cases + any configurable amount of random values.

fedefernandez · 2023-12-01T10:47:03Z

evaluator/src/test/kotlin/com/xebia/functional/xef/evaluator/SuiteBuilderSpec.kt

+          outputResponse { emptyOutputResponse }
+        }
+      }
+      .shouldBeLeft() shouldBe listOf<ValidationError>(EmptyItemSpecOutputResponse(1))


Same as above. I prefer the way ItemSpecBuilderSpec defines the data, but here we can also group some tests in a single one.

fedefernandez · 2023-12-01T10:48:41Z

evaluator/src/test/kotlin/com/xebia/functional/xef/evaluator/utils/Generators.kt

+
+object Generators {
+  val simpleString = Arb.string(1, 60, Codepoint.az())
+  val emptyString = Arb.stringPattern("\\s+")


I think this could be probably more efficient to generate a random-size list of one of the blank spaces

fedefernandez · 2023-12-01T10:49:12Z

evaluator/src/test/kotlin/com/xebia/functional/xef/evaluator/TestModelsSpec.kt

+import kotlin.test.Test
+import kotlinx.coroutines.test.runTest
+
+class TestModelsSpec {


These can me coded as a property-based tests

tomascayuelas · 2023-12-01T12:00:05Z

Thank you very much @fedefernandez for your comments/review.
As we have already spoken by slack, I take care of them, I think they are very good, and can be better code. Thank you!

raulraja · 2024-03-05T12:49:47Z

@fedefernandez @tomascayuelas is this still relevant?

nomisRev

We're here using Kotest inside Kotlin Test 🤕 Never seen that before 😅 Why are we using both?

We can also define a simple DSL by ourselves that covers the requirements of this PR. It's probably not much more than what we've already defined on top of Kotest.

Additionally, I agree with @fedefernandez that we should either properly do prop testing (with random/configurable SEED), or use fixed data.

A couple small questions, thoughts, and concerns on some the domain classes defined in this PR.

nomisRev · 2024-03-05T14:19:16Z

evaluator/src/main/kotlin/com/xebia/functional/xef/evaluator/EitherOps.kt

+import kotlinx.serialization.encodeToString
+import kotlinx.serialization.json.Json
+
+object EitherOps {


No, it's a pretty bad practice sadly. It doesn't play well with auto-complete for imports, in Kotlin.
These should just be top-level function, when we get proper namespaces in Kotlin 2.x this will probably improve but for now they need to be top-level.

nomisRev · 2024-03-05T14:20:59Z

evaluator/src/main/kotlin/com/xebia/functional/xef/evaluator/ItemSpecBuilder.kt

+  suspend fun contextDescription(block: suspend () -> String) =
+    contexts.add(ContextDescription.invoke { block() })
+
+  suspend fun outputResponse(block: suspend () -> String) =
+    outputs.add(OutputResponse.invoke { block() })


Are these functions ever called in parallel? If yes then MutableList will result in race conditions here. Use Atomic<List<_>> to fix.

nomisRev · 2024-03-05T14:21:40Z

evaluator/src/main/kotlin/com/xebia/functional/xef/evaluator/ItemSpecBuilder.kt

+      { ensure(input.isNotBlank()) { EmptyItemSpecInput } },
+      {
+        mapOrAccumulate(contexts.withIndex()) { nameAndIndex ->
+            nameAndIndex.value.recover { error -> raise(error) }.bind()


What is .recover { error -> raise(error) } doing here?

nomisRev · 2024-03-05T14:26:23Z

evaluator/src/main/kotlin/com/xebia/functional/xef/evaluator/models/ContextDescription.kt

+    suspend operator fun invoke(
+      block: suspend () -> String


Suggested change

suspend operator fun invoke(

block: suspend () -> String

inline operator fun invoke(

block: () -> String

Why is this suspend?

nomisRev · 2024-03-05T14:26:33Z

evaluator/src/main/kotlin/com/xebia/functional/xef/evaluator/models/ContextDescription.kt

+@JvmInline
+value class ContextDescription(val value: String) {
+  companion object {
+    @JvmSynthetic


Why is @JvmSynthetic being added here?

nomisRev · 2024-03-05T14:26:57Z

evaluator/src/main/kotlin/com/xebia/functional/xef/evaluator/models/ContextDescription.kt

+import com.xebia.functional.xef.evaluator.models.errors.EmptyContextDescription
+
+@JvmInline
+value class ContextDescription(val value: String) {


Should this constructor be private to prevent ContextDescription to be created with an empty string?
Is this because it needs to be parsed from JSON using reflection? 😭

nomisRev · 2024-03-05T14:29:46Z

evaluator/src/test/kotlin/com/xebia/functional/xef/evaluator/EitherOpsSpec.kt

+class EitherOpsSpec {
+  private val validSuiteSpec = runBlocking {
+    SuiteSpec(simpleString.next()) {
+      outputDescription { simpleString.next() }


I see we're using Kotest here, we can potentially piggy back on their SEED which is random and printed on failure.

Otherwise I would at least work with a SEED, and print it somewhere on each build, or use fixed data like @fedefernandez mentioned. I am not a fan of next() at all, since it's quite unpredictable.

nomisRev · 2024-03-05T14:31:59Z

evaluator/src/test/kotlin/com/xebia/functional/xef/evaluator/ItemSpecBuilderSpec.kt

+  }
+
+  @Test
+  fun shouldInvalidWhenAnyCombinationOfInputAreEmpty() = runTest {


Should we look into property testing? It's supported by Kotest out-of-the-box, and has some cool stuff like Exhaustive. In contrast to Arb it only defines "edge cases", and makes sure that by minimum the full cartesian product of all edge cases are run by the tests.

That's pretty cool, since it can be used to run the least amount of tests but still cover all edge cases + any configurable amount of random values.

fedefernandez · 2024-03-05T17:40:27Z

@fedefernandez @tomascayuelas is this still relevant?

@raulraja I feel it's. I can take a look and prepare for reviewing

We're here using Kotest inside Kotlin Test 🤕 Never seen that before 😅 Why are we using both?

@nomisRev We faced some binary incompatibilities and found the issues you created in Arrow. We're using the kotlin test framework but kotest for the assertions.

arrow-kt/arrow#3183

nomisRev · 2024-03-13T07:48:52Z

I see I am blocking this PR @fedefernandez.

To give some context on Arrow, that was to avoid rewriting more code and to political continue supporting Kotest. We replaced the Kotest runner only because we wanted to support WASM, and be able to build EAP releases (as part of the community builds).

However, I don't want to block this PR on that matter. I do feel we can simplify some things here, Junit also support "property based testing" in the form of table based testing. Not sure if Qwik works for KMP, but I've used it for kotlin-test on JVM.

That leaves the same concerns as you originally raised @fedefernandez concerning testing practices. I am removing my request for changes however to unblock this PR from my side.

fedefernandez · 2024-04-01T08:40:53Z

Thanks @nomisRev. I have already started resolving some comments; I'll get back to you once I have something.

fedefernandez · 2024-04-14T16:14:20Z

I'm afraid I don't have enough context to fix the conflicts. Therefore, I vote to close the PR.

tomascayuelas requested a review from javipacheco November 29, 2023 11:02

tomascayuelas self-assigned this Nov 29, 2023

javipacheco previously approved these changes Nov 30, 2023

View reviewed changes

evaluator-example/src/main/kotlin/com/xebia/functional/xef/evaluator/examples/TestExample.kt Show resolved Hide resolved

fedefernandez reviewed Nov 30, 2023

View reviewed changes

fedefernandez previously approved these changes Nov 30, 2023

View reviewed changes

tomascayuelas and others added 3 commits November 30, 2023 15:24

First iteration to add tests and some functions to perform content va…

19254a5

…lidations.

Change from funcional to functional

a7e5604

Adding test for EitherOps functions

Apply spotless formatting

33fd96c

tomascayuelas dismissed stale reviews from fedefernandez and javipacheco via 33fd96c November 30, 2023 14:40

tomascayuelas force-pushed the xef-evaluator-improvements branch from 57ff093 to 33fd96c Compare November 30, 2023 14:40

Apply spotless formatting

85444a9

tomascayuelas requested review from javipacheco and fedefernandez December 1, 2023 10:23

Remove kotest framework and use kotlin test runner

a763d4d

tomascayuelas force-pushed the xef-evaluator-improvements branch from bed6f58 to a763d4d Compare December 1, 2023 10:27

fedefernandez reviewed Dec 1, 2023

View reviewed changes

javipacheco and others added 2 commits January 4, 2024 16:26

Merge branch 'main' into xef-evaluator-improvements

a811a44

Merge branch 'main' into xef-evaluator-improvements

632caf7

Merge branch 'main' into xef-evaluator-improvements

82c134c

nomisRev requested changes Mar 5, 2024

View reviewed changes

nomisRev and others added 2 commits March 6, 2024 18:46

Merge branch 'main' into xef-evaluator-improvements

36e3b9d

Merge branch 'main' into xef-evaluator-improvements

5316c98

nomisRev self-requested a review March 13, 2024 07:48

javipacheco closed this Apr 15, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[xef-evaluator] - Improvements #568

[xef-evaluator] - Improvements #568

tomascayuelas commented Nov 29, 2023 •

edited

Loading

javipacheco left a comment

fedefernandez left a comment

tomascayuelas commented Nov 30, 2023

fedefernandez commented Nov 30, 2023

tomascayuelas commented Nov 30, 2023

fedefernandez left a comment

fedefernandez Dec 1, 2023

nomisRev Mar 5, 2024

fedefernandez Dec 1, 2023

nomisRev Mar 5, 2024

fedefernandez Dec 1, 2023

nomisRev Mar 5, 2024

fedefernandez Dec 1, 2023

fedefernandez Dec 1, 2023

fedefernandez Dec 1, 2023

tomascayuelas commented Dec 1, 2023

raulraja commented Mar 5, 2024

nomisRev left a comment

nomisRev Mar 5, 2024

nomisRev Mar 5, 2024

nomisRev Mar 5, 2024

nomisRev Mar 5, 2024

nomisRev Mar 5, 2024

nomisRev Mar 5, 2024

nomisRev Mar 5, 2024

nomisRev Mar 5, 2024

fedefernandez commented Mar 5, 2024 •

edited

Loading

nomisRev commented Mar 13, 2024

fedefernandez commented Apr 1, 2024

fedefernandez commented Apr 14, 2024

[xef-evaluator] - Improvements #568

[xef-evaluator] - Improvements #568

Conversation

tomascayuelas commented Nov 29, 2023 • edited Loading

javipacheco left a comment

Choose a reason for hiding this comment

fedefernandez left a comment

Choose a reason for hiding this comment

tomascayuelas commented Nov 30, 2023

fedefernandez commented Nov 30, 2023

tomascayuelas commented Nov 30, 2023

fedefernandez left a 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

tomascayuelas commented Dec 1, 2023

raulraja commented Mar 5, 2024

nomisRev left a 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

fedefernandez commented Mar 5, 2024 • edited Loading

nomisRev commented Mar 13, 2024

fedefernandez commented Apr 1, 2024

fedefernandez commented Apr 14, 2024

tomascayuelas commented Nov 29, 2023 •

edited

Loading

fedefernandez commented Mar 5, 2024 •

edited

Loading