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

#52 - add support for null in should clause #80

Closed

Conversation

alex-bogomaz
Copy link
Contributor

hi, please review this PR. it contains fix for issue #52.
Now should be null should work the same as should be Null

@@ -11,12 +11,29 @@ type ``be Null tests`` ()=

[<Test>] member test.
``null should fail to not be Null`` ()=
null |> should be Null
shouldFail (fun () -> null |> should not' (be Null))
Copy link
Member

Choose a reason for hiding this comment

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

Could we save prev behavior for backward compatibility as well?

null |> should be Null
null |> should be null  

Copy link
Contributor Author

Choose a reason for hiding this comment

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

first 2 test of the test-class have exact same body, but different naming:

  • null should be Null
  • null should fail to not be Null

So they really test same thing.
There is the same situation in be Null tests for XUnit and MsTest.
If you take a loot at NUnit tests you'll see that first 2 tests are testing what is stated in their names:

[<Test>] member test.
     ``null should be Null`` ()=
        null |> should be Null

    [<Test>] member test.
     ``null should fail to not be Null`` ()=
        shouldFail (fun () -> null |> should not' (be Null))

So I guess there should be no backward-compatibility issues and now test body corresponds to the test name, just like similar test for NUnit

@alex-bogomaz
Copy link
Contributor Author

PR is updated. Could you please review and provide feedback.

As for back-compatibility: both null |> should be Null and null |> should be null are valid assertions. So previous behaviour is still valid. There are unit-tests for this.

PR has 3 commits - if this doesn't work for you, I'll create new PR with single commit

@sergey-tihon
Copy link
Member

@alex-bogomaz Could we close this PR?

@alex-bogomaz
Copy link
Contributor Author

yes

sergey-tihon added a commit that referenced this pull request Dec 7, 2015
Fix of #52 - add support for null in should (replacement for PR #80)
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.

2 participants