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

Fix test services for addding service reference #1611

Merged
merged 1 commit into from
Oct 25, 2016

Conversation

hongdai
Copy link
Contributor

@hongdai hongdai commented Oct 24, 2016

  • We would like to leverge the same test server for WCF connected service
    testing.
  • I have verified the updated test service can be used for adding service references, both http and net.tcp
    bindings.

* We would like to leverge the same test server for WCF connected service
testing.
@hongdai hongdai changed the title Fix test services for addding service reference - Do not review yet, testing for *nix Fix test services for addding service reference Oct 25, 2016
@hongdai
Copy link
Contributor Author

hongdai commented Oct 25, 2016

Ready for review.

@@ -15,7 +15,6 @@ namespace MessageContractCommon
public class MessageContractConstants
{
// CONSTANTS
public const string wrapperName = "CustomWrapperName";
Copy link
Contributor

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.

Copy link
Contributor Author

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")]
Copy link
Contributor

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.

Copy link
Contributor

@roncain roncain left a 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.

@hongdai hongdai merged commit 12d528c into dotnet:master Oct 25, 2016
@hongdai hongdai deleted the FixWebService branch October 26, 2016 20:05
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.

3 participants