re 826: I've got together a bit of a work in progr...

# kotest-contributors

[dr.dreigh]

re 826: I've got together a bit of a work in progress diff, but I'm a little stalled in places where I could do with a bit of help knowing where to put things, and how config works.

This is what I have got so far on 826:
https://github.com/kotest/kotest/compare/master...jaymoid:826-help-needed?expand=1

It prints diffs like:

data class diff for com.sksamuel.kotest.eq.DataClass3
├ x: expected:<99> but was:<88>
├ y: data class diff for com.sksamuel.kotest.eq.DataClass2
│  ├ x: expected:<2> but was:<1>
│  ├ y: expected:<4.4f> but was:<3.4f>
│  └ z: data class diff for com.sksamuel.kotest.eq.DataClass1
│     ├ a: expected:<99> but was:<2>
│     └ b: expected:<7.6f> but was:<4.6f>
└ z: expected:<44.6> but was:<44.4>

I have got as far as I can on my own, there's quite a few bits in the code where I could do with a bit of help, they are marked TODO, or just HELP! 🙂

• I've added Any?.isDataClass function to return true the instance is a data class and if the platform supports reflection. I'm not sure I've added all the implementations in the right places, or if I actually needed them all? (Non JVM kotlin is new to me!)
• Because this feature is not useful if the user decides to override equals() in their data class, I've added a propertyConfig value showDetailedDataClassDiff - however I'm not sure if a) the best way of accessing it form the DataClassEq.kt class? and b) if this flag should be on a per spec basis rather than project? (or both?)
• I imagine that a very deeply nested data class would cause this code to stack overflow.
• I'm not sure where DataClassEq.kt should live because it contains platform dependent code.
Any help/suggestions/code changes/feedback would be greatly appreciated. Cheers!

[sam]

Is it rebased to current master

[dr.dreigh]

yeah, I pulled from upstream.

[dr.dreigh]

Just noticed that newline, on extensions.kt, will fix shortly [edit] fixed[/edit]

[sam]

I created a PR so I could add some comments

[dr.dreigh]

Cool, thanks - plenty to be getting on with 🙂

[sam]

Sure is. If you can finish it next couple of days, we can include it in 4.1.0-RC1

[sam]

Otherwise we can bump it to 4.2

[dr.dreigh]

Right, I'll get my skates on 🙂

[dr.dreigh]

Hi, I did a bit of work on this last night, if you get a min can you check it's going in the right direction please?

I think I understand the reflection stuff now, I've added a function to the reflection interface to get the members declared in the primary constructor. Not sure if this was the right thing to do? If it is, I think the next step is preventing it exposing the KCallable type as non-JVM platforms don't have this and is causing some failing builds from platforms without this type.

[dr.dreigh]

It's now building, but not added any features to hide it behind a flag yet.

[sam]

Will check tonight

[dr.dreigh]

^ thanks for the feedback, I've added that config obj and had a bit of a tidy. 👍

[sam]

Is it good to be merged ?

[dr.dreigh]

haha - err I think it's very close 😬 let me know what you think - also happy to raise a new PR with commits squished

[sam]

I can squash when merging

[dr.dreigh]

Hi, there's a couple of failing tests when run on ubuntu - they pass on my machine 🤣

They are:

Test shouldTimeout should pass if a coroutine takes longer than the given timeout[jvm] FAILED
Test executionError[jvm] FAILED

Wondering if they are bit flakey because they are timing based? Not sure I've done anything to affect that area.

[sam]

They're new, and have pretty tight timings so yeah that'll be why

[dr.dreigh]

aaah ok, cool

[sam]

The options you moved to the config object

[sam]

did you leave typealiases in place ?

[sam]

never mind I see they were just sysprops before

[dr.dreigh]

ah yeah - is that ok? Thought I'd try and move those into one place

[sam]

no that's good

[dr.dreigh]

ah you've merged it - thanks 🙂