Contributing - unit tests fixes and code analysis warnings

[itzik kasovitch]

Hi,

I am trying to contribute to the project. The first thing I tried is downloading the code and build it. 

The build project fails since some of the unit tests fail on my machine. The main reason is related to the fact that I am on a different locale so exception messages are not the same as the unit test expects. Another thing I noticed is that the code analysis check found a couple of warnings. Now I am happy to fix that but I have some questions first:

1. Am I expected to create an issue for that - one for the unit tests and another for the code analysis warnings?
2. What are the project requirements with regard to localization?
3. What are the project requirements with regard to code analysis warnings?
4. On the project documentation I can't find a prerequisites section to help new contributors get up to speed. I am happy to fix that to but my obvious question is do I need to open an issue for this too?

Best regards,

ikasovitch

[Damian Schenkelman]

Hi Itzik,

Please find my answers inline.

Anyone else feel free to chime in.

Thanks,

On Sat, Mar 29, 2014 at 8:22 AM, itzik kasovitch <ikaso...@gmail.com> wrote:

Hi,

I am trying to contribute to the project. The first thing I tried is downloading the code and build it. 

The build project fails since some of the unit tests fail on my machine. The main reason is related to the fact that I am on a different locale so exception messages are not the same as the unit test expects. Another thing I noticed is that the code analysis check found a couple of warnings. Now I am happy to fix that but I have some questions first:

1. Am I expected to create an issue for that - one for the unit tests and another for the code analysis warnings?

Regarding unit tests failing due to exception messages: unless the fix is fairly simple (i.e. using InvariantCulture) you should probably leave it as is. Localization is not a priority at the moment. If everything else is working correctly, the tests will pass in the build server. 

Regarding code analysis: I don't think the build fails when running build.cmd and finding CA issues. Unless you see an obvious way that could cause the CA issue to lead to an error you should leave it as is. 

1. What are the project requirements with regard to localization?

Not a priority, see above. 

1. What are the project requirements with regard to code analysis warnings?

I don't think the build fails. They are there as a warning so we can use our judgement to determine if it merits a fix or not. 

1. On the project documentation I can't find a prerequisites section to help new contributors get up to speed. I am happy to fix that to but my obvious question is do I need to open an issue for this too?

Could you please be more specific about what you mean by "up to speed"? We have CONTRIBUTING.md which usually leads to issues being created, discussing the issues and eventually a PR.

 

Best regards,

ikasovitch

[itzik kasovitch]

Hi Daniel,

Thanks for the quick response. I have spent 0.5 hour to see the reason for the failing unit tests and one unit test fix was easy another failed because of a missing expectation on a mock object and not due to culture issues. So I think that those fixes should be checked in. Anyway you did not answer my question re the requirement to create an issue for fixing unit tests.

You are right about the fact that code analysis warnings are not failing the build. Nevertheless, if those warnings are not failing the build isn't it better to turn them off? They just create noise in the build output.

To be more specific about the prerequisites I can give 2 examples:

• VS 2012
• xUnit Test Runner for VS 2012

But this is not my point. I just wanted to clarify if changing the Contributing.md document requires a dedicated issue.

Itzik

[itzik kasovitch]

Of course I meant Damian and not Daniel. :)

[Damian Schenkelman]

Inline

On Sat, Mar 29, 2014 at 9:30 AM, itzik kasovitch <ikaso...@gmail.com> wrote:

Hi Daniel,

Thanks for the quick response. I have spent 0.5 hour to see the reason for the failing unit tests and one unit test fix was easy another failed because of a missing expectation on a mock object and not due to culture issues. So I think that those fixes should be checked in. Anyway you did not answer my question re the requirement to create an issue for fixing unit tests.

That is weird, they should be failing in the build server. As this seems to be a bug please create an issue and the submit a PR for it (if you are willing to :)).

You are right about the fact that code analysis warnings are not failing the build. Nevertheless, if those warnings are not failing the build isn't it better to turn them off? They just create noise in the build output.

Personally I don't mind the warnings but I can understand why they could seem like noise. I like the fact that CA runs (I might forget to run it from VS) and some errors might actually be worth fixing.

Perhaps Justin (who put together most of our build infrastructure if I recall correctly) can shed more detail on this.

 

To be more specific about the prerequisites I can give 2 examples:

• VS 2012
• xUnit Test Runner for VS 2012

Oh, I see that would be nice. The particular ones you mentioned are not mandatory pre-requisites though (you could also use VS 2013 and run the tests from build.cmd), but having a section with possible versions of VS to use, link to Roslyn latest CTP and package and also suggestions for the test runner would be nice IMO.

 

But this is not my point. I just wanted to clarify if changing the Contributing.md document requires a dedicated issue.

If you submit the PR we can take a look at it directly. 

Itzik

[itzik kasovitch]

Thanks Damian,

Now its clear! :)

Of course I am willing too. I will prepare a PR for the unit tests and go on.

Itzik