Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Iterable test #2960

Merged
merged 11 commits into from
Mar 9, 2023
Merged

Iterable test #2960

merged 11 commits into from
Mar 9, 2023

Conversation

abendt
Copy link
Contributor

@abendt abendt commented Mar 6, 2023

this adds a couple of tests to IterableTest to improve test coverage for functions in Iterable

#2894

abendt added 2 commits March 6, 2023 22:59
this adds a couple of tests to IterableTest to improve test coverage for functions in Iterable
@abendt abendt mentioned this pull request Mar 6, 2023
9 tasks
@abendt
Copy link
Contributor Author

abendt commented Mar 7, 2023

i think there is a bug in the implentation of unalign. How i understand it align should be the inverse of unalign: align(unalign xs) shouldBe xs.

However this only works if all xs have both values. Then the current implementation of unalign will split the input into two list of equal length:

val xs = listOf(Both(1,x), Both(2,y), Both(3,z))
val (l,r) = xs.unalign() // listOf(1,2,3) to listOf(x,y,z)
l.align(r) shouldBe xs 

with this its possible to align both lists and to get back to the initial value of xs.

If xs contains Left or Right values then the splitted lists will have differing length. This leads to elements end up in a wrong position in the list:

val xs = listOf(Left(1), Both(2,y), Right(z))
val (l,r) = xs.unalign() // listOf(1,2) to listOf(y,z)
l.align(r) shouldBe xs // wrong! actual value is listOf(Both(1,y), Both(2,z))

One way to fix that could be to let unalign return Pair<List<A?>, List<B?>>. Missing elements in the source would then be represented as null in the result

@nomisRev
Copy link
Member

nomisRev commented Mar 7, 2023

Hmm, interesting find @abendt. Thank you, great work!

This would be breaking, and would thus need to land in 2.0.0. In a PR I am preparing for some deprecations for Iterable.kt I actually wanted to rename unalign to seperateIor because it's behavior matches seperateEither and not unalign as you mention here.

Not sure what is the best path forward though 🤔 I think it would be best to create a separate PR against the arrow-2 branch changing the unalign signature, and adding these tests there. Then we can update this PR with the seperateIor introduction, and relevant tests. Feel free to take this change from my WIP into to your PR:

public fun <A, B> Iterable<Ior<A, B>>.seperateIor(): Pair<List<A>, List<B>> =
  fold(emptyList<A>() to emptyList()) { (l, r), x ->
    x.fold(
      { l + it to r },
      { l to r + it },
      { a, b -> l + a to r + b }
    )
  }

@Deprecated(
  "The current unalign function is renamed to seperateIor, and a new unalign function is going to be added to Arrow 2.0.0.",
  ReplaceWith("seperateIor()", "arrow.core.seperateIor")
)
public fun <A, B> Iterable<Ior<A, B>>.unalign(): Pair<List<A>, List<B>> = seperateIor()

WDYT? Are you interested in adding such a PR against arrow-2? And making the other relevant changes here?

@nomisRev nomisRev requested a review from serras March 7, 2023 10:11
@abendt
Copy link
Contributor Author

abendt commented Mar 7, 2023

yes, i will make the changes here and create a new PR

@nomisRev
Copy link
Member

nomisRev commented Mar 7, 2023

Awesome, thank you so much @abendt 🙏 👏 👏

abendt added 2 commits March 7, 2023 13:21
deprecate unalign function
introduce separateIor function
@abendt
Copy link
Contributor Author

abendt commented Mar 7, 2023

i have reverted the generator implementation so the tests are passing again.
also added the deprecations as discussed (also added a deprecation for the 2nd unalign function, as we have the same problem there)

@abendt
Copy link
Contributor Author

abendt commented Mar 7, 2023

hey @nomisRev, some of the gitlab checks fail because of timeouts. think this is not related to my changes? any idea how to resolve them?

@nomisRev
Copy link
Member

nomisRev commented Mar 7, 2023

It's unrelated to your changes @abendt, it's been a long standing issue on Gradle side and the fix has finally been merged and released in 7.6. I created a PR to bump the timeout on CI. #2965

@nomisRev nomisRev requested review from franciscodr, a team and gutiory March 7, 2023 19:02
* revert exception type
* remove test that tests stdlib `zip` function
Copy link
Member

@nomisRev nomisRev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome work @abendt! 🙌 🥳 Thank you 🙏

@nomisRev nomisRev merged commit a1fffdb into arrow-kt:main Mar 9, 2023
@abendt abendt deleted the iterable_test branch March 9, 2023 08:55
abendt added a commit to abendt/arrow that referenced this pull request Mar 9, 2023
@abendt abendt mentioned this pull request Mar 9, 2023
serras added a commit that referenced this pull request Mar 24, 2023
see discussion in
#2960 (comment)
#2960 (comment)

---------

Co-authored-by: Alejandro Serrano <[email protected]>
nomisRev added a commit that referenced this pull request Apr 24, 2024
* The beginning of Arrow 2.0

* Resource Arrow 2.0 (#2786)

* Flatten Resource ADT, maintain API

* [Arrow 2.0] Effect without suspending shift (#2797)

* Shift without suspend, inline all the rest
* Add new error handlers signatures
* Make ShiftCancellationException private

* Rename Shift to Raise according to Slack Poll, and add some initial docs (#2827)

* Remove all references to shift from new Arrow 2.0 code (#2834)

* Remove all references to shift from new code

* Update API files

* Fixes merge conflict between main and arrow-2 (#2835)

* Add Resource.allocated() to decompose Resource into it's allocate and… (#2820)
* [2743] Migrate internal use of CircuitBreaker double to duration (#2748)
* Fix typo (#2824)
* Make the server disconnect test more general (#2822)
* Update NonEmptyList.fromList deprecation to suggest `toOption()` instead (#2832)
* Improve Either.getOrHandle() docs (#2833)
* Improve allocated, and fix knit examples
Co-authored-by: Jeff Martin <[email protected]>
Co-authored-by: Martin Moore <[email protected]>
Co-authored-by: valery1707 <[email protected]>
Co-authored-by: Lukasz Kalnik <[email protected]>
Co-authored-by: stylianosgakis <[email protected]>

* Add Atomic module, and StateShift (#2817)

* Two small deprecations

* Add Atomic module, and StateShift. Implement ior through StateShift

* Fix build

* Fix atomic knit

* Fix knit attempt #2

* Update API files

* Remove references to shift

* Change return type of raise to Nothing (#2839)

* Change return type of raise to Nothing

Implements #2805

* Update public api with ./gradlew apiDump

* Suppress warnings for unreachable calls to fail in tests

The test is verifying that no code will be executed after a call to
raise, so it's actually testing the very behaviour that the compiler is
warning us about.

Co-authored-by: Alejandro Serrano <[email protected]>

* Update API files

* Increase timeout

* Fix compiler bug with nested inline + while + return

* Clean up ExitCase.fromError

* Update API files@

* Feature/remove validated (#2795)

Co-authored-by: Simon Vergauwen <[email protected]>
Co-authored-by: Alejandro Serrano <[email protected]>

* Remove CancellationExceptionNoTrace.kt after merge, and run apiDump

* Add missing runnerJUnit5 for arrow-atomic JVM

* Publish Arrow 2.0.0-SNAPSHOT (#2871)

Co-authored-by: Javier Segovia Córdoba <[email protected]>

* Simplify optics to Traversal/Optional/Lens/Prism (#2873)

* 'mapOrAccumulate' for Raise (#2872)

Co-authored-by: Simon Vergauwen <[email protected]>

* Fix problems with Atomic

* Smaller timeouts

* Remove Tuple10 to Tuple22, Const, Eval, computation blocks, and arrow-continuations (#2784)

* Revert typo

* Fix build

* Fix ParMapJvmTest

* Implement NonEmptyList using value class (#2911)

* Fix merge w.r.t. Saga

* apiDump

* Test other return expression

* change unalign signature (#2972)

see discussion in
#2960 (comment)
#2960 (comment)

---------

Co-authored-by: Alejandro Serrano <[email protected]>

* Update after merge main

* Fix :arrow-optics-ksp-plugin:compileKotlin

* Fix Every instances

* Move functions to arrow functions (#3014)

* Bring back `Iso` (#3013)

* Bring back Iso

* API Dump

---------

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

* Update BOM (#3019)

* Fix andThen duplication (#3020)

* Fix Knit

* Fix weird problem with value classes

* Update API docs

* Update publish workflow

Following the instructions in #3090 (comment)

* No closing repo on snapshot

* knit

* Fix optics tests

* Fix after merge

* Refactor ParMapTest from Kotest Plugin to Kotlin-test runtime #3191 (#3221)

Co-authored-by: Simon Vergauwen <[email protected]>
Co-authored-by: Alejandro Serrano <[email protected]>

* Refactor: Use Kotlin-test runtime for arrow-fx-stm tests (#3224)

Co-authored-by: Alejandro Serrano <[email protected]>

* Update all Gradle files to mention kotlin.test

* Refactor ParZip2Test from Kotest Plugin to Kotlin-test runtime #3192 (#3222)

Co-authored-by: Alejandro Serrano <[email protected]>

* Refactor ParZip3Test from Kotest Plugin to Kotlin-test runtime #3193 (#3223)

Co-authored-by: Alejandro Serrano <[email protected]>

* Refactor GuaranteeCaseTest to use kotlin test (#3226)

Closes #3190

* refactor: migrate NotEmptySetTest to kotlin-test (#3230)

Co-authored-by: Alejandro Serrano <[email protected]>

* refactor: migrate EagerEffectSpec to kotlin-test (#3233)

Co-authored-by: Alejandro Serrano <[email protected]>

* Refactor NullableSpec from Kotest Plugin to Kotlin-test runtime (#3236)

#3153

Co-authored-by: Alejandro Serrano <[email protected]>

* Refactor BracketCaseTest to use kotlin test (#3237)

Closes #3186

Co-authored-by: Alejandro Serrano <[email protected]>

* Move arrow-functions tests to kotlin.test (#3243)

* Inline `AtomicBoolean` (#3240)

* Inline AtomicBoolean

* Update API files

---------

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

* refactor: migrate MappersSpec to kotlin-test (#3248)

* Refactor ResourceTestJvm from Kotest Plugin to Kotlin-test runtime (#3244)

Closes #3213

* refactor: migrate FlowJvmTest to Kotlin-test (#3228)

* Refactor ParZip9JvmTest from Kotest Plugin to Kotlin-test runtime (#3245)

Closes #3211

* Refactor ParZip8JvmTest from Kotest Plugin to Kotlin-test runtime (#3246)

Closes #3210

* refactor: migrate NumberInstancesTest to kotlin-test (#3232)

* refactor: OptionTest to kotlin-test runtime (#3229)

* Revert "Inline `AtomicBoolean` (#3240)" (#3279)

This reverts commit a6f1e73.

* Refactor ParZip6JvmTest from Kotest Plugin to Kotlin-test runtime (#3255)

Closes #3208

* Refactor ParZip5JvmTest from Kotest Plugin to Kotlin-test runtime (#3256)

Closes #3207

* Refactor ParZip3JvmTest from Kotest Plugin to Kotlin-test runtime (#3258)

Closes #3205

* Refactor ParZip2JvmTest from Kotest Plugin to Kotlin-test runtime (#3259)

Closes #3204

* Refactor ParMapJvmTest from Kotest Plugin to Kotlin-test runtime (#3260)

Closes #3203

* Refactor ParZip4JvmTest from Kotest Plugin to Kotlin-test runtime (#3257)

Closes #3206

* refactor: migrate RaiseAccumulateSpec to kotlin-test (#3250)

* Refactor ParZip7JvmTest from Kotest Plugin to Kotlin-test runtime (#3247)

Closes #3209

Co-authored-by: Alejandro Serrano <[email protected]>

* Update ComparisonKtTest.kt (#3274)

Co-authored-by: Simon Vergauwen <[email protected]>

* Update OptionSpec.kt (#3271)

Co-authored-by: Simon Vergauwen <[email protected]>

* Update TraceJvmSpec.kt (#3276)

* Update TraceJvmSpec.kt

* Update TraceJvmSpec.kt

---------

Co-authored-by: Simon Vergauwen <[email protected]>

* Update ParZip9Test.kt (#3265)

* Update ParZip9Test.kt

* Update ParZip9Test.kt

* Update ParZip8Test.kt (#3266)

* Update ParZip7Test.kt (#3267)

* Update ParZip6Test.kt (#3268)

* Update ParZip5Test.kt (#3269)

* Update ParZip4Test.kt (#3270)

* Refactor CountDownLatchSpec and CyclicBarrierSpec to use kotlin test (#3227)

* Refactor CountDownLatchSpec to use kotlin test

Closes #3187

* Refactor CyclicBarrierSpec to use kotlin test

Closes #3188

---------

Co-authored-by: Alejandro Serrano <[email protected]>

* Refactor NonEmptyListTest to kotlin-test (#3231)

* Add kotlin test dependency

* Refactor NonEmptyList Test to use kotlin test

---------

Co-authored-by: Alejandro Serrano <[email protected]>

* refactor: migrate EffectSpec to kotlin-test (#3234)

* Refactor FlowTest to use kotlin test (#3238)

Closes #3189

Co-authored-by: Alejandro Serrano <[email protected]>

* refactor: migrate IorSpec to kotlin-test (#3249)

Co-authored-by: Simon Vergauwen <[email protected]>

* refactor: migrate ResultSpec to kotlin-test (#3251)

Co-authored-by: Simon Vergauwen <[email protected]>

* refactor: migrate StructuredConcurrencySpec to kotlin-test (#3252)

* refactor: migrate TraceSpec to kotlin-test (#3253)

* refactor: migrate GeneratorsTest to kotlin-test (#3254)

* Refactor RaceNJvmTest from Kotest Plugin to Kotlin-test runtime (#3261)

Closes #3212

Co-authored-by: Alejandro Serrano <[email protected]>

* Update ArrowResponseEAdapterTest.kt (#3264)

* Update ArrowResponseEAdapterTest.kt

* Update ArrowResponseEAdapterTest.kt

---------

Co-authored-by: Simon Vergauwen <[email protected]>
Co-authored-by: Alejandro Serrano <[email protected]>

* Update CollectionsSyntaxTests.kt (#3273)

* Update CollectionsSyntaxTests.kt

* Update CollectionsSyntaxTests.kt

---------

Co-authored-by: Simon Vergauwen <[email protected]>
Co-authored-by: Alejandro Serrano <[email protected]>

* Update NonFatalJvmTest.kt (#3277)

* Update ArrowEitherCallAdapterTest.kt (#3278)

* Update ArrowEitherCallAdapterTest.kt

* Update ArrowEitherCallAdapterTest.kt

* Move tests from `serialization` and `functions` completely to `kotlin.test` (#3289)

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

* Fix problems with tests

* Remove a bunch of warnings in `arrow-2` (#3282)

Co-authored-by: serras <[email protected]>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

* Finish transition to `kotlin.test` of `retrofit` and `fx-coroutines` (#3291)

* Fix problems with concurrency in tests, take 8

* Port rest of `arrow-core` to `kotlin.test` (#3292)

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

* Implement `fixedRate` using monotonic time source (#3294)

Co-authored-by: serras <[email protected]>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

* Port `optics` tests to `kotlin.test` (#3295)

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

* Add or fix contracts in Raise (#3293)

* Add or fix contracts in Raise

* Make contracts less strict

* Get back contract on Either

* ignoreErrors should also be AT_MOST_ONCE

---------

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

* Alternate `SingletonRaise` (#3328)

Co-authored-by: Youssef Shoaib <[email protected]>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: serras <[email protected]>

* Remove (unused) tests for high-arity module

* Fix tests + Knit

* Fix merge NullableSpec

* Regression in Arb.list?

* Fix test for nonEmptyList

* Develocity warning

* Fix merge problem with optics-ksp-plugin

* Fix timeout in test

---------

Co-authored-by: Simon Vergauwen <[email protected]>
Co-authored-by: Sam Cooper <[email protected]>
Co-authored-by: Marc Moreno Ferrer <[email protected]>
Co-authored-by: Javier Segovia Córdoba <[email protected]>
Co-authored-by: Alphonse Bendt <[email protected]>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: Andreas Storesund Madsen <[email protected]>
Co-authored-by: molikuner <[email protected]>
Co-authored-by: Jonathan Lagneaux <[email protected]>
Co-authored-by: Marcus Ilgner <[email protected]>
Co-authored-by: Chris Black <[email protected]>
Co-authored-by: Tejas Mate <[email protected]>
Co-authored-by: HyunWoo Lee (Nunu Lee) <[email protected]>
Co-authored-by: serras <[email protected]>
Co-authored-by: Youssef Shoaib <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants