nblockchain / fsx

FSX is the ideal tool for people that use F# for their scripting needs.
MIT License
14 stars 5 forks source link

Fsdk,Tests: added FSharpUtil module and tests #17

Closed webwarrior-ws closed 1 year ago

webwarrior-ws commented 1 year ago

Moved FSharpUtil module from GWallet. Added Fsdk.Tests Nunit test project and moved FSharpUtil there.

webwarrior-ws commented 1 year ago

Not sure about #if NoDUsAsStructs part thouogh. Right now it's just dead code.

knocte commented 1 year ago

CI is red btw

knocte commented 1 year ago

And you need to make sure CI runs the tests, otherwise if you just add them like this, they are not being run.

knocte commented 1 year ago

Not sure about #if NoDUsAsStructs part thouogh. Right now it's just dead code.

The thing is, you said that ResultUtils was using FSharpUtil's Either type, or something like that, and it isn't. Just bring the Either type and don't bring any ResultUtils stuff, there's no need to bring that.

knocte commented 1 year ago

@webwarrior-ws this error:

/Users/runner/.dotnet/sdk/7.0.101/FSharp/Microsoft.FSharp.Targets(224,9): error MSB3191: Unable to create directory "obj/Debug/net6.0/". Access to the path '/Users/runner/work/fsx/fsx/Fsdk.Tests/obj/Debug' is denied. [/Users/runner/work/fsx/fsx/Fsdk.Tests/Fsdk.Tests.fsproj]

Is likely being caused by the fact that the make install step is being run with sudo, which causes the binaries to be generated with root as owner of them; and then after that you're running the unit tests without sudo.

You might argue that CI shouldn't run a build with sudo permissions, and you might be right about that. However, make install phase needs sudo permissions in order to copy the binaries to a prefix that is owned by root (e.g. /usr). We could fix this by running make first without sudo, and make install later with sudo; however, that change should probably go to another PR. To side-step this problem in this PR, let's just run the unit tests before make install, simple and effective, and this way you don't need to do any chmod calls.

knocte commented 1 year ago

I see some CI runs fail because of this flaky test:

A total of 1 test files matched the specified pattern.
  Failed basic test for WhenAnyAndAll [2 s]
  Error Message:
     Expected: greater than 00:00:02
  But was:  00:00:01.9979935

I in fact detected this and brought awareness about it here: https://github.com/nblockchain/geewallet/commit/ff4ad70b097d4639fca6ea2e30873d175bb796a5 . @webwarrior-ws can you address the FIXMEs in this PR? By improving the documentation you will gain a better understanding of that function, which in turn will hopefully make you be able to understand how to fix the test to not be flakey anymore 🙏

knocte commented 1 year ago

The compiler error /__w/fsx/fsx/Fsdk/FSharpUtil.fs(201,38): error FS0039: The field, constructor or member 'Choice' is not defined I think happens because FSharp.Core version needs to be raised.

webwarrior-ws commented 1 year ago

I in fact detected this and brought awareness about it here: nblockchain/geewallet@ff4ad70 . @webwarrior-ws can you address the FIXMEs in this PR? By improving the documentation you will gain a better understanding of that function, which in turn will hopefully make you be able to understand how to fix the test to not be flakey anymore 🙏

I added documentation. Both function and test seem to be correct. Given the numbers, I think that's just timing error.

knocte commented 1 year ago

I think that's just timing error.

Define "timing error"?

webwarrior-ws commented 1 year ago

I think that's just timing error.

Define "timing error"?

Maybe Async.Sleep is not that precise in terms of time. So sometimes it finishes a couple of milliseconds earlier or later.

knocte commented 1 year ago

Maybe Async.Sleep is not that precise in terms of time. So sometimes it finishes a couple of milliseconds earlier or later.

If you're right, then it should be easy to tweak the problematic Sleep call with something higher or lower, to make the test not flakey.

knocte commented 1 year ago

And this compiler error:

 /__w/fsx/fsx/Fsdk.Tests/FSharpUtil.fs(6,6): error FS0039: The namespace or module 'NUnit' is not defined. [/__w/fsx/fsx/Fsdk.Tests/Fsdk.Tests-legacy.fsproj]

is maybe because a nuget restore call is missing. Not sure if there's any nuget restore at the moment in master branch (legacy .NET) TBH.

knocte commented 1 year ago

@webwarrior-ws I fixed the red CI (sorry, it was my bad to recommend you to run fsharpi in legacy scenario, because I was forgetting that it would fail in the stockmono lanes; now I've fixed it properly).

I'm merging this now so a new Fsdk package will get published in nuget. With it, tomorrow you can create PR for geewallet that removes FSharpUtil code and uses this new package (and in the case of NOnion, you can do it in a new commit to be added in this PR from yours that is already open: https://github.com/nblockchain/NOnion/pull/55 ).

Another thing you can do tomorrow after the above is create a PR to move the stuff in test/testTsv.fsx to Fsdk.Tests project please.