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

Making tests pass using NCrunch #7

Closed
wants to merge 3 commits into from

Conversation

AlbertoMonteiro
Copy link
Contributor

I use NCrunch as test runner, and some test that are based on path like MetricsCalculationPerformanceTest they were breaking because NCrunch doesn't use same directory that solution are in.

@jjrdk
Copy link
Owner

jjrdk commented Feb 9, 2015

There's a little too much going on in this PR. As far as I can see the only NCrunch related change is where you set the current directory in the performance test.

Honestly I'd prefer that you keep the NCrunch related stuff on a separate branch as it may not be relevant for a lot of consumers.

@AlbertoMonteiro
Copy link
Contributor Author

Hi Jacob, sorry but I didn't understand this: There's a little too much going on in this PR.
Can you explain me?


About NCrunch related changes, you are right, I just change the current directory because NCrunch doesn't use assemblies/code in project folder, it copies to another location.


But about not relevant for a lot of consumers I didn't understand.
What do you call consurmers? People who use this by nuget package Roslyn Code Analysis?
If so, these people will not be affected. Since you doesn't deploy any related test stuff.


The question of relevance is debatable, you put some powershell scripts to do a custom build, which is related code updating the nuget package, I being a possible contributor, do not think that is relevant to me.

People who want to download source code and do something, will just download more one package, the "NCrunch.Framework" package, that is just 155 KB (159.565 bytes).

@jjrdk
Copy link
Owner

jjrdk commented Feb 9, 2015

What I mean by 'too much going on' is that there are major changes to the project files (almost complete replacement, according to the diff viewer) without any noticeable reason. The .gitignore file also has a significant amount of changes which I can only assume relate to the files generated by NCrunch.

More importantly is the fact that you wish to include NCrunch into the main branch. NCrunch is a commercial tool. On top of the 155KB you mention it also requires a license. This makes it inappropriate for a code base which is intended to be free and open source. This is why I suggest that you maintain a separate NCrunch branch for yourself.

@AlbertoMonteiro
Copy link
Contributor Author

I asked the owner of NCrunch about license, and he said that:


Legally there isn't an issue with including the changes for NCrunch (and the reference to NCrunch.Framework). NCrunch is a productivity tool and is licensed as such .. it isn't a commercial library.

If referencing NCrunch.Framework in the project file is creating some friction, know that you can avoid this by simply writing the NCrunch.Framework.Environment.GetOriginalProjectPath() method in your own code. I've included the source code for this method below:

public static string GetOriginalProjectPath()
{
    return Environment.GetEnvironmentVariable("NCrunch.OriginalProjectPath");
}

If I commt some modification like that below, would you allow?

[SetUp]
public void Setup()
{
    _calculator = new ProjectMetricsCalculator(new CodeMetricsCalculator());
#if NCRUNCH
    var directoryName = System.IO.Path.Combine(System.IO.Path.GetDirectoryName(GetOriginalProjectPath()), "bin", "Debug");
    System.IO.Directory.SetCurrentDirectory(directoryName); 
#endif
}

#if NCRUNCH
public static string GetOriginalProjectPath()
{
    return Environment.GetEnvironmentVariable("NCrunch.OriginalProjectPath");
} 
#endif

@AlbertoMonteiro
Copy link
Contributor Author

@jjrdk any comment?

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