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.

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

Add typed aliases around Xunit #203

Merged
merged 26 commits into from
Apr 6, 2022
Merged

Add typed aliases around Xunit #203

merged 26 commits into from
Apr 6, 2022

Conversation

Smaug123
Copy link
Contributor

I'm flying totally blind here; FAKE doesn't work on Apple Silicon until fsprojects/FAKE#2626 is fixed, so I can't build. Relying on the pipelines for everything.

Copy link
Member

@sergey-tihon sergey-tihon left a comment

Choose a reason for hiding this comment

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

Thank you @Smaug123 !
@CaptnCodr what do you think?

We need to fix #204 before merge

@sergey-tihon sergey-tihon self-requested a review April 3, 2022 18:52
Copy link
Member

@sergey-tihon sergey-tihon left a comment

Choose a reason for hiding this comment

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

@Smaug123 in F# we still need to list all *.fs file in *.fsproj (it does not happed automatically because order is matter)

You need to modify two files
https://github.com/fsprojects/FsUnit/blob/master/src/FsUnit.Xunit/FsUnit.Xunit.fsproj#L16
https://github.com/fsprojects/FsUnit/blob/master/tests/FsUnit.Xunit.Test/FsUnit.Xunit.Test.fsproj#L38
otherwise you code does not compile and will not be included in assembly

@Smaug123
Copy link
Contributor Author

Smaug123 commented Apr 3, 2022

facepalm thanks - this is what happens when the tooling prevents me from building or using an IDE locally!

@Smaug123
Copy link
Contributor Author

Smaug123 commented Apr 3, 2022

(If someone who can actually build the thing could do this, that would be amazing - I think it's the work of five minutes to fix the tests with an IDE.)

@sergey-tihon
Copy link
Member

I'm flying totally blind here; FAKE doesn't work on Apple Silicon until fsprojects/FAKE#2626 is fixed, so I can't build. Relying on the pipelines for everything.

@Smaug123 Fake does not let you use build.sh but what does not work in IDE?

@Smaug123
Copy link
Contributor Author

Smaug123 commented Apr 3, 2022

Oh, I can run Paket without FAKE - of course! Sorry, I'm new to the FAKE/Paket ecosystem and hadn't really grokked that they were different tools.

tests/FsUnit.Xunit.Test/typed.shouldBeSmallerThanTests.fs Outdated Show resolved Hide resolved
tests/FsUnit.Xunit.Test/typed.shouldBeSmallerThanTests.fs Outdated Show resolved Hide resolved
tests/FsUnit.Xunit.Test/typed.shouldEqualTests.fs Outdated Show resolved Hide resolved
tests/FsUnit.Xunit.Test/typed.shouldEqualTests.fs Outdated Show resolved Hide resolved
src/FsUnit.Xunit/FsUnitTyped.fs Outdated Show resolved Hide resolved
src/FsUnit.Xunit/FsUnitTyped.fs Outdated Show resolved Hide resolved
@Smaug123
Copy link
Contributor Author

Smaug123 commented Apr 3, 2022

@sergey-tihon It's late at night for me so I'm mostly asleep, but I think this has uncovered an inconsistency between the XUnit and NUnit versions. NUnit shouldEqual (and should equal) uses the equality instance on the first argument; XUnit should equal uses the equality instance on the second argument. I've pushed some tests (in 70e3c4b) that demonstrate the inconsistency; they are copy-pasted from the equivalent NUnit tests.

How would you like to resolve it? The obvious answer would be to swap the equality in the CustomMatcher equal for XUnit, but this is a breaking change in semantics. The alternative is to accept (and document) that the semantics of should equal are different in the two frameworks.

/// The equality instance on `expected` is used, if available.
[<DebuggerStepThrough>]
let shouldNotEqual<'a> (expected: 'a) (actual: 'a) =
should not' (equal actual) expected
Copy link
Member

Choose a reason for hiding this comment

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

actual and exoected swapped here as well

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, this was what happened when I was trying to fix a failing test - but it turned out not to be because I'd got the arguments in the wrong order, but instead because of the inconsistent behaviour of should equal between NUnit and Xunit.

@sergey-tihon
Copy link
Member

I do not see inconsistency (at least for now), can you please explain it to me?

FsUnit follow actual |> shouldX expected pattern in conditions, so all functions written as let shouldX expected actual =. FsUnit should underlying framework (NUnit, xUnit) assertion and correctly pass expected to expected in order to produce correct err rot messages.

NUnit shouldEqual looks correct for me.
image

as well as should
image

@Smaug123
Copy link
Contributor Author

Smaug123 commented Apr 4, 2022

This test behaves differently in xUnit vs NUnit:

    [<Fact>]
    member __.``should pass when Equals returns true``() =
        anObj |> should equal (box(new AlwaysEqual()))

(but on master, that test is not present in the Xunit tests.)

In xUnit, a |> should equal b ultimately (via the CustomMatcher) asks whether a = b; that is, whether a.Equals(b).

In NUnit, a |> should equal b asks whether a Equality.IsEqualTo(b), which boils down to Is.EqualTo(b), which NUnit interprets as _.AreEqual(b, a), which ultimately asks whether b.Equals(a).

The semantics of the equal CustomMatcher we define for XUnit are therefore not the same as the semantics of NUnit.

@Smaug123
Copy link
Contributor Author

Smaug123 commented Apr 4, 2022

I'll put them in the right order again, document and raise a bug for the issue, and continue with this PR without considering changing the semantics.

@Smaug123
Copy link
Contributor Author

Smaug123 commented Apr 4, 2022

At long last, I now consider this PR to be ready.

src/FsUnit.Xunit/FsUnitTyped.fs Outdated Show resolved Hide resolved
src/FsUnit.Xunit/FsUnitTyped.fs Outdated Show resolved Hide resolved
src/FsUnit.Xunit/FsUnitTyped.fs Outdated Show resolved Hide resolved
src/FsUnit.Xunit/FsUnitTyped.fs Outdated Show resolved Hide resolved
src/FsUnit.Xunit/FsUnitTyped.fs Outdated Show resolved Hide resolved
@CaptnCodr
Copy link
Member

Looks good after corrections.

@Smaug123
Copy link
Contributor Author

Smaug123 commented Apr 4, 2022

Looks good after corrections.

I can make the changes, but just to check, you've requested no semantic differences?

@CaptnCodr
Copy link
Member

CaptnCodr commented Apr 4, 2022

You also could probably use the Assert methods as in FsUnit.NUnit/FsUnitTyped.fs e.g.:
NUnit: Assert.IsEmpty(list)
xUnit: Assert.Empty(list).

I liked that way as it was in the first commits for short circuiting to xUnit. (not into CustomMatcher)
Now, semantics are ok for me.

@Smaug123
Copy link
Contributor Author

Smaug123 commented Apr 5, 2022

@sergey-tihon Would you mind taking another look at this please?

# Conflicts:
#	src/FsUnit.NUnit/FsUnit.fs
#	src/FsUnit.Xunit/CustomMatchers.fs
@sergey-tihon
Copy link
Member

@Smaug123 may I ask you to update test? We unified behavior between NUnit and xUnit and always call equality check on expected rather than actual

@Smaug123
Copy link
Contributor Author

Smaug123 commented Apr 5, 2022

In that case I need to swap the order of arguments to be inconsistent with Nunit, or make a breaking change to the semantics of should equal in Xunit. Which would you prefer?

@sergey-tihon
Copy link
Member

@Smaug123 I believe that we already did breaking change to the semantics of should equal in Xunit
changes are in master and in your pr branch.

@sergey-tihon sergey-tihon merged commit 736b7a1 into fsprojects:master Apr 6, 2022
@sergey-tihon
Copy link
Member

Thank you @Smaug123 !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants