-
Notifications
You must be signed in to change notification settings - Fork 565
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
Fix test services for addding service reference #1611
Conversation
* We would like to leverge the same test server for WCF connected service testing.
d15c946
to
12f0fab
Compare
Ready for review. |
@@ -15,7 +15,6 @@ namespace MessageContractCommon | |||
public class MessageContractConstants | |||
{ | |||
// CONSTANTS | |||
public const string wrapperName = "CustomWrapperName"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: could you have just changed the constant here instead of removing it? The rest of the tests use these constants to avoid duplicating strings inside the tests. Not blocking.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer not to have a const defined in the common code for wrapper name as it could be different per test, although it happens to be the same for the existing 3 tests.
@@ -78,7 +76,7 @@ public interface IMessageContract | |||
|
|||
public class MessageContractTypes | |||
{ | |||
[MessageContract(IsWrapped = true, WrapperName = "CustomWrapperName", WrapperNamespace = "http://www.contoso.com")] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[No change required -- observation only] It's too bad the original test didn't use the same constants in MessageContractConstants for these attribute named parameters. It would have eliminated so much string duplication scattered in the contracts and tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
A small non-blocking nit to consider.
testing.
bindings.