Fixing issue #1709 #1711
Fixing issue #1709 #1711
Conversation
There was a problem hiding this comment.
Excellent, @asgerhallas, thanks! I have no comments about the code itself.
Just two remarks:
- We've introduced a few breaking changes on
masternot long ago, which means the next release frommasterwill be 6.0.0. We're not quite ready to release it yet, though, which means your changes won't be released for a while if we merge them tomaster. If you'd like to see it released sooner, could you please rebase your work onto thesupport/5.xbranch and submit another PR to that branch? We'll then release a minor 5.x version from there. - The
FakeItEasy.Testsproject is mostly legacy. We try to move away from "low-level" unit tests and usually avoid adding new tests to this project.; we use BDD specs instead (see the FakeItEasy.Specs project). Would you mind changing your tests to specs? (in theFakeSpecsclass, I think)
There was a problem hiding this comment.
Thanks, @asgerhallas. I made a few comments to add onto @thomaslevesque's. It may seem like a lot, but they're generally quite small adjustments I'm asking for. (And some comments may even evaporate when the tests move to specs.)
Overall, I quite enjoyed your changes.
tests/FakeItEasy.Tests/FakeTests.cs
Outdated
| @@ -1,3 +1,6 @@ | |||
| using FakeItEasy.Core; | |||
| using FluentAssertions; | |||
There was a problem hiding this comment.
Probably won't be an issue once the tests get moved to Specs as @thomaslevesque has requested, but our convention is to put usings inside the namespace, like using System below.
There was a problem hiding this comment.
Still need to move those inside the namespace
Ha, I was a little surprised by the sparseness of those FakeTests... this explains it :) |
There was a problem hiding this comment.
Hi @asgerhallas,
Thanks for the changes. Almost there, just a few more minor comments!
tests/FakeItEasy.Specs/FakeSpecs.cs
Outdated
| "Given a fake object" | ||
| .x(() => fake = A.Fake<object>()); | ||
|
|
||
| "And I try to get the FakeManager for that object" |
There was a problem hiding this comment.
"And" => "When"
Specs follow the Given-When-Then pattern
tests/FakeItEasy.Specs/FakeSpecs.cs
Outdated
| "Given a non-fake object" | ||
| .x(() => notFake = new object()); | ||
|
|
||
| "And I try to get the FakeManager for that object" |
tests/FakeItEasy.Specs/FakeSpecs.cs
Outdated
| "Given a fake object" | ||
| .x(() => fake = A.Fake<object>()); | ||
|
|
||
| "And I check if that object is fake" |
tests/FakeItEasy.Tests/FakeTests.cs
Outdated
| @@ -1,3 +1,6 @@ | |||
| using FakeItEasy.Core; | |||
| using FluentAssertions; | |||
There was a problem hiding this comment.
Still need to move those inside the namespace
No description provided.