Suggestion: follow the "Arrange Act Assert" style for tests

[zvirja]

Currently our tests follow the clumsy Four-Phase Test style to describe the test body. I don't like that at all, as it looks like some old-school stuff which doesn't play nice with xUnit. My concerns:

• The // Teardown phase is always empty in xUnit tests as it creates a sut per test, without need to restore it.
• The // Fixture setup, // Exercise System and // Verify Outcome looks too verbose, while // arrange, // act and // assert are more laconic. If you don't create test from a template, it's much simpler to write those words (it's always hard to write exercise without typos 😅).

Therefore, @moodmosaic if you don't have any strong objections (I hope you don't), I would like to switch us to 3 phase tests and use it all our the tests. It will allow me to feel satisfaction during the test creation, rather than pain 😖

[moodmosaic]

Go for it. But I'd prefer to have the first letter capital, that is

// Arrange
// Act
// Assert

instead of

// arrange
// act
// assert

Do you have strong reasons to prefer the second case?

[zvirja]

No, uppercase is fine to me - it's enough that we use AAA 😊

[moodmosaic]

You can also separate each phase with a blank line, which is the formatting I use nowadays. Then, the test-reader can assume it's Given/When/Then, or Arrange/Act/Assert, or even (the four-phase one goes here).

You can read more about this in the section "Simple tests with more than three lines of code" here.

[zvirja]

I strongly dislike usage of empty lines instead of comments in OSS and disagree with that Marks idea. See my comment here.

[moodmosaic]

Comments on top of each test-phase are, basically, comments. And in this particular case, they are apologies, because they explain how —instead of why.

---

Now, back to AutoFixture, since all tests are decorated with comments, and since the fixture teardown phase is almost all the times empty, we can go ahead and change them to AAA.

[zvirja]

And in this particular case, they are apologies

Well, I also refer to this rule often, however in this particular case I prefer to keep those "apologizes" 😄 They make structure of the test more obvious and strict. Personally I find it more easier to read "labeled" tests, but probably that is simply a matter of habit 😉

[moodmosaic]

but probably that is simply a matter of habit

This; and perhaps also a matter of who actually wrote the test.

[micheleissa]

Hello, I'd like to tackle this. I'm a big fan of Autofixture and use it in my work all the time and it would be a good way of giving back.
So basically you guys want to change the following in all the test files:
// Fixture setup to // Arrange
// Exercise System to // Act
// Verify Outcome to // Assert
and finally // Teardown to nothing.

[moodmosaic]

Yes, but keep in mind that doing a 'find and replace' might not work as some phases are joined together in some tests. That is, you might come across Exercise system & verify outcome and similar, IIRC.

[micheleissa]

@moodmosaic I understand, I'm planning to do file by file not a full 'find and replace' but I might still use it in the more obvious/simpler cases.

Thank you,
Eissa

[micheleissa]

@moodmosaic for // Exercise system and verify outcome it should it be replaced with
// Act and Assert
Or you guys thinking of something different?

[zvirja]

Usually I use // Act & Assert as it looks more concise and cool 😎 Unless @moodmosaic has concerns, I'd prefer to follow that style 😉

[moodmosaic]

Either way is OK

[micheleissa]

So I'm trying to start on this and after I load the All.sln in VS 2017 two F# projects
( AutoFoq,AutoFoqUnitTest ) are failing to open. I have zeor experience with F# so any help would be appreciated

[zvirja]

@micheleissa Ensure that you use latest version of VS (15.5.1 or later), otherwise project will not open (MS applied changes to their F# SDK). Also ensure to run git clean -fdx . being in root with closed VS. That is to cleanup the temporary files in case you checked out the project earlier.

Both those steps should help. Let me know if they didn't 😉

[micheleissa]

@zvirja I have upgraded to latest version of VS 2017 15.5.2 and still facing the same issue 😞

[zvirja]

Have you tried the second part?

• Close VS
• Checkout the latest master
• Go to root and run git clean -fdx .
• Open VS again?

It seems that you simply have leftovers from the previous SDK.

Also ensure that F# tooling is installed on your machine:

[micheleissa]

Yea, I forgot the second one sorry :)
I'll try it now

[micheleissa]

@zvirja that worked, Thanks a lot for helping out!

[micheleissa]

One more question, how do I associate the branch i'm working on in my forked repo to this ticket?

[zvirja]

Great that it worked! 👍

how do I associate the branch i'm working on in my forked repo to this ticket?

I'd suggest to look at this article, the workflow is well described there: https://github.com/sevntu-checkstyle/sevntu.checkstyle/wiki/Development-workflow-with-Git:-Fork,-Branching,-Commits,-and-Pull-Request

Basically, you don't need to associate a branch with this issue. Rather, you create a Pull Request and simply mention this issue in the body (e.g. see this one).

[micheleissa]

Ok, got it. Thank you! I'm gonna create the pull request while I'm working on the issue so you guys can monitor the progress and give feedback according to what mentioned in the contributing guidelines.