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.

Sign up for GitHub

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

Jump to bottom

Fsdk,Tests: added FSharpUtil module and tests #17

Merged
merged 2 commits into from
Jan 15, 2023
Merged

Fsdk,Tests: added FSharpUtil module and tests #17

merged 2 commits into from
Jan 15, 2023

Conversation

webwarrior-ws
Copy link
Contributor

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

@webwarrior-ws
Copy link
Contributor Author

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

knocte
knocte reviewed Jan 9, 2023
View reviewed changes
Fsdk/FSharpUtil.fs Outdated

module FSharpUtil =

module ReflectionlessPrint =
Copy link
Member

Choose a reason for hiding this comment

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

@webwarrior-ws this is related to STRICTER_COMPILATION_BUT_WITH_REFLECTION_AT_RUNTIME , I thought there's no need to bring this code here

Copy link
Member

Choose a reason for hiding this comment

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

s/thought/told you

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So just delete all SPrintF* functions and tests?

Copy link
Member

@knocte knocte Jan 9, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

yes; but those were not the only tests, you need to bring the tests in AsyncExtensions.fs (you even worked on them last week!), and AsyncCancellation.fs, and any that are about FSharpUtil please, I don't understand why you didn't make sure to bring them before making the PR

knocte
knocte reviewed Jan 9, 2023
View reviewed changes
Fsdk/FSharpUtil.fs Outdated

let option = OptionBuilder()

let Retry<'T, 'TException when 'TException :> Exception> sourceFunc retryCount: Async<'T> =
Copy link
Member

Choose a reason for hiding this comment

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

I see this is not the Retry func from NOnion, is this one better? and will it work with NOnion?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This one is more generic (NOnion one only works with Async<unit>), and also NOnion's uses TorLogger.

Copy link
Member

Choose a reason for hiding this comment

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

ok cool; I assume we can refactor NOnion later to use this Retry from Fsdk?

@knocte
Copy link
Member

knocte commented Jan 9, 2023

CI is red btw

@knocte
Copy link
Member

knocte commented Jan 9, 2023

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

@knocte
Copy link
Member

knocte commented Jan 10, 2023

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.

@webwarrior-ws webwarrior-ws force-pushed the fsharputil branch 24 times, most recently from 6e867d2 to 4b5eb97 Compare January 11, 2023 13:04
knocte
knocte reviewed Jan 12, 2023
View reviewed changes
Fsdk/Fsdk-legacy.fsproj Outdated
@@ -50,6 +50,7 @@
<Reference Include="System.Web.Extensions" />
</ItemGroup>
<ItemGroup>
<Compile Include="FSharpUtil.fs" />
Copy link
Member

Choose a reason for hiding this comment

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

@webwarrior-ws nit, please move it one position below AssemblyInfo.fs

@knocte
Copy link
Member

knocte commented Jan 12, 2023

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: 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 🙏

@knocte
Copy link
Member

knocte commented Jan 12, 2023

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 webwarrior-ws force-pushed the fsharputil branch 5 times, most recently from 0cb6dbe to 1f9257f Compare January 12, 2023 10:53
@webwarrior-ws
Copy link
Contributor Author

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
Copy link
Member

knocte commented Jan 12, 2023

I think that's just timing error.

Define "timing error"?

@webwarrior-ws
Copy link
Contributor Author

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
Copy link
Member

knocte commented Jan 12, 2023

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
Copy link
Member

knocte commented Jan 12, 2023

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.

@webwarrior-ws webwarrior-ws force-pushed the fsharputil branch 8 times, most recently from b683357 to 29272e0 Compare January 12, 2023 13:24
@knocte knocte force-pushed the fsharputil branch 4 times, most recently from 2618318 to 480d3d2 Compare January 15, 2023 11:52
webwarrior-ws and others added 2 commits January 15, 2023 21:12
@webwarrior-ws @knocte
Fsdk: added FSharpUtil module and tests
b07af48 
Moved FSharpUtil module from geewallet and added Fsdk.Tests
NUnit test project and moved tests for FSharpUtil there.

Co-authored-by: Andres G. Aragoneses <[email protected]>
@webwarrior-ws @knocte
Fsdk.Tests: fix flaky WhenAnyAndAll test
2809b9f 
Introduced error margin for minimal run time in
"basic test for WhenAnyAndAll" to eliminate random failures.
@knocte
Copy link
Member

knocte commented Jan 15, 2023

@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: nblockchain/NOnion#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.

@knocte knocte merged commit 2550578 into nblockchain:master Jan 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@knocte knocte knocte left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@webwarrior-ws @knocte