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

Add command-line option to specify configuration file #348

Merged
merged 3 commits into from
Jan 13, 2018
Merged

Conversation

CharliePoole
Copy link
Collaborator

Fixes #246

Adds the option to the console runner. The underlying capability has been there for some time and is already used by the NUnit 3 VS adapter.

mikkelbu
mikkelbu previously approved these changes Jan 12, 2018
Copy link
Member

@mikkelbu mikkelbu left a comment

Choose a reason for hiding this comment

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

Looks Good To Me 👍

Assert.That(AppDomain.CurrentDomain.SetupInformation.ConfigurationFile, Is.SamePath(expectedPath));
var expectedPath = Path.Combine(TestContext.CurrentContext.TestDirectory, STANDARD_CONFIG_FILE);
var alternatePath = Path.Combine(TestContext.CurrentContext.TestDirectory, ALTERNATE_CONFIG_FILE);
Assert.That(AppDomain.CurrentDomain.SetupInformation.ConfigurationFile, Is.SamePath(expectedPath).Or.SamePath(alternatePath));
Copy link
Member

Choose a reason for hiding this comment

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

Would you mind explaining the changes in this file to me? I'm a bit confused where alt.config has come from?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good catch... I should add alt.config in GitHub rather than directly in my bin directory where it is, of course, being ignored.

Basically, alt.config is a standard way of manually testing that the command-line option works. By running the tests with --configfile=alt.config I am able to verify the complete chain of events that cause the correct file to be read. The changes here are so that existing tests do not get broken when I do that.

This is really not absolutely necessary because we have unit tests that verify each individual link in the chain but it made me more confident to be able to add it.

I think it will be clearer once I actually add the file.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks - that made sense. 😄 Could I suggest a comment to cover that - that the alternatePath part of these two tests is only in the case where a --configFile option has been used manually?

Copy link
Member

Choose a reason for hiding this comment

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

I completely overlooked the tests when I reviewed (so much for multitasking between tasks).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@ChrisMaddock I'll do that. Things like this are always "obvious" to the guy who writes them!

Copy link
Member

@ChrisMaddock ChrisMaddock left a comment

Choose a reason for hiding this comment

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

LGTM - Waiting on CI

@CharliePoole
Copy link
Collaborator Author

Still having problem with Apple agents not starting. Overriding to merge.

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.

None yet

3 participants