Remove the FsToolkit.ErrorHandling.TaskResult dependency from the main test project to enable full backwards compat testing with FSharp.Core 6.0.1
#181
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
bartelink
reviewed
Oct 28, 2023
bartelink
reviewed
Oct 28, 2023
src/FSharp.Control.TaskSeq.Test/FSharp.Control.TaskSeq.Test.fsproj
Outdated
Show resolved
Hide resolved
…skResult dependency We want to remove the dependency in our tests on FsToolkit.ErrorHandling in PR #181 because it uses a high version of FSharp.Core. In other words, it disallows us to properly test TaskSeq with the lowest-denominator FSharp.Core it was compiled with, which we should do, to ensure stability in the widest range of forward-and-backward compatibility. The SmokeTests library, on the other hand, uses the highest RTM release of FSharp.Core and we don't really care about other deps there.
…er used `TaskResult` in these files anyway We want to remove the dependency in our tests on FsToolkit.ErrorHandling.TaskResult in PR #181 because it uses a high version of FSharp.Core. In other words, it disallows us to properly test TaskSeq with the lowest-denominator FSharp.Core it was compiled with, which we should do, to ensure stability in the widest range of forward-and-backward compatibility. The SmokeTests library, on the other hand, uses the highest RTM release of FSharp.Core and we don't really care about other deps there.
…rom the `TaskSeq.Test` project We want to remove the dependency in our tests on FsToolkit.ErrorHandling.TaskResult in PR #181 because it uses a high version of FSharp.Core. In other words, it disallows us to properly test TaskSeq with the lowest-denominator FSharp.Core it was compiled with, which we should do, to ensure stability in the widest range of forward-and-backward compatibility. The SmokeTests library, on the other hand, uses the highest RTM release of FSharp.Core and we don't really care about other deps there.
…: 6.0.1: achieve best backward compat testing We want to remove the dependency in our tests on FsToolkit.ErrorHandling.TaskResult in PR #181 because it uses a high version of FSharp.Core. In other words, it disallows us to properly test TaskSeq with the lowest-denominator FSharp.Core it was compiled with, which we should do, to ensure stability in the widest range of forward-and-backward compatibility. The SmokeTests library, on the other hand, uses the highest RTM release of FSharp.Core and we don't really care about other deps there.
abelbraaksma
force-pushed
the
remove-fstoolkit-taskresult-from-tests
branch
from
cee3082 to
32c57b3
Compare
Member
Author
|
Thanks for review @bartelink!! 👍 I've updated the code based on your suggestions and will merge to |
abelbraaksma
added
dependencies
bartelink
reviewed
Oct 29, 2023
Member
Author
|
@bartelink it looks like you don't have write access, so your review is not unblocking (but I can unblock myself, of course). Maybe I should give you write access? |
abelbraaksma
added a commit
that referenced
this pull request
Oct 29, 2023
…skResult dependency We want to remove the dependency in our tests on FsToolkit.ErrorHandling in PR #181 because it uses a high version of FSharp.Core. In other words, it disallows us to properly test TaskSeq with the lowest-denominator FSharp.Core it was compiled with, which we should do, to ensure stability in the widest range of forward-and-backward compatibility. The SmokeTests library, on the other hand, uses the highest RTM release of FSharp.Core and we don't really care about other deps there.
abelbraaksma
added a commit
that referenced
this pull request
Oct 29, 2023
…er used `TaskResult` in these files anyway We want to remove the dependency in our tests on FsToolkit.ErrorHandling.TaskResult in PR #181 because it uses a high version of FSharp.Core. In other words, it disallows us to properly test TaskSeq with the lowest-denominator FSharp.Core it was compiled with, which we should do, to ensure stability in the widest range of forward-and-backward compatibility. The SmokeTests library, on the other hand, uses the highest RTM release of FSharp.Core and we don't really care about other deps there.
abelbraaksma
added a commit
that referenced
this pull request
Oct 29, 2023
…rom the `TaskSeq.Test` project We want to remove the dependency in our tests on FsToolkit.ErrorHandling.TaskResult in PR #181 because it uses a high version of FSharp.Core. In other words, it disallows us to properly test TaskSeq with the lowest-denominator FSharp.Core it was compiled with, which we should do, to ensure stability in the widest range of forward-and-backward compatibility. The SmokeTests library, on the other hand, uses the highest RTM release of FSharp.Core and we don't really care about other deps there.
abelbraaksma
added a commit
that referenced
this pull request
Oct 29, 2023
…: 6.0.1: achieve best backward compat testing We want to remove the dependency in our tests on FsToolkit.ErrorHandling.TaskResult in PR #181 because it uses a high version of FSharp.Core. In other words, it disallows us to properly test TaskSeq with the lowest-denominator FSharp.Core it was compiled with, which we should do, to ensure stability in the widest range of forward-and-backward compatibility. The SmokeTests library, on the other hand, uses the highest RTM release of FSharp.Core and we don't really care about other deps there.
Member
Apologies; please put me on the drive by commenters list ;) |
Member
Author
|
@bartelink I've given you |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Problem statement
The library
FsToolkit.ErrorHandling.TaskResultis an excellent library, but recently they have introduced non backwards-compatible dependencies that limit our ability to continue using this library in our tests for obvious reasons: we want the tests to be as close to real-world as possible, using the lowest denominator FSharp.Core that theTaskSeqlibrary supports.Since
TaskSeqis backward compatible till FSharp.Core 6.0.1, we should try to test it with that version.Simple solution
The simplest way is to move the dependent tests to SmokeTests (which is only the PoC, ironically showing the problems with lists of Tasks using an oversight of the TaskResult library, where it seems to support lists of Tasks, but not really if they're hot-started, and they always are).
Note that SmokeTests does the opposite, and tests the library, using a a pull of the actual NuGet lib, against recent versions of FSharp.Core.