Avoid NotSerializableException when TryValues or EitherValues raise assertion failure

[seblm]

Fixes #1883

[cla-bot]

Hi @seblm, we require contributors to sign our Contributor License Agreement, and we don't have yours on file. In order for us to review and merge your code, please access https://www.artima.com/cla/choose-type to sign our Contributor License Agreement. Your effort is highly appreciated. Thank you.

[artimasites]

@cla-bot[bot] check

[cla-bot]

The cla-bot has been summoned, and re-checked this pull request!

[kpbochenek]

@seblm if you add custom serialization shouldn't you also add custom deserializer? Won't there be problems on side receiving this event?

Moving PatienceConfiguration to not be inner class would be too big change right?

[seblm]

@seblm if you add custom serialization shouldn't you also add custom deserializer? Won't there be problems on side receiving this event?

You've right: serializing then deserializing produce an error. I'll fix it.

Moving PatienceConfiguration to not be inner class would be too big change right?

Unfortunately, this change will not be binary compatible. I think it should be introduced in a major or a minor version instead of patch version.

[seblm]

@seblm if you add custom serialization shouldn't you also add custom deserializer? Won't there be problems on side receiving this event?

You've right: serializing then deserializing produce an error. I'll fix it.

With a proxy which is not an inner class, serialization of PatienceConfig is ok. @kpbochenek you can have a look.

[kpbochenek]

@seblm if you add custom serialization shouldn't you also add custom deserializer? Won't there be problems on side receiving this event?

You've right: serializing then deserializing produce an error. I'll fix it.

With a proxy which is not an inner class, serialization of PatienceConfig is ok. @kpbochenek you can have a look.

I don't know much about java serialization, as long as it works 👍

Just hope it can be merged and released quickly 🙏

[cheeseng]

@seblm @kpbochenek Sorry for the late response, has been away for other works. I am looking into this now.

[cheeseng]

@seblm @kpbochenek This PR unfortunately is having problem under scala-js and dotty build, I have submitted the following PR to your clone:

https://github.com/seblm/scalatest/pull/1

Can you sanity check if the fix works for you?

Note that I have run MIMA test for this PR + New PR, seems to pass. Hopefully we can release a new release containing this fix soon.

[kpbochenek]

Thanks for looking into it!
I will try to find a time and try it but it might not be that easy, tests show problem very well so if they pass I think that should be enough 👍

[seblm]

Thanks a lot @cheeseng for your pull request. It is now rebased onto my fork.
I was not aware of scala-js and dotty exceptions.

[cheeseng]

@bvenners This PR looks good to me now, and it passed MIMA test, do you mind to review for merging when it is convenient for you? Do you mind if we release 3.2.3 and 3.1.5 for this?

Cheers.

[bvenners]

Why is this protected[concurrent] instead of just private[concurrent]?

[seblm]

You've right: this visibility can be reduced to private. The change is applied.

[bvenners]

Tnx. Testing this now.

[bvenners]

Merged. Thanks very much for this PR!

[seblm]

Thank you to have merge it so quickly.

Do you think that it could be possible to drop jdk object serialization usage from scalatest?

[bvenners]

@seblm Say you added SerialVersionUID annotations to two classes in this PR. Can you elaborate on why you thought that was important? I would think we may not want to give those, so they'll be generated from the class interface. Otherwise I think we have to pay attention to when we change it incompatibly, which we would likely miss. Tnx.

[seblm]

@bvenners you have right on this: I added SerialVersionUID more by habit than by real conviction. It is ok to remove these annotations for the sake of maintainability.

[seblm]

I am glad to see that #2086 will handle this problem more deeply 👍