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

TaskWebClientTest.UploadValues_Success test is failing #79731

Closed
wfurt opened this issue Dec 16, 2022 · 5 comments · Fixed by #79975
Closed

TaskWebClientTest.UploadValues_Success test is failing #79731

wfurt opened this issue Dec 16, 2022 · 5 comments · Fixed by #79975
Labels
area-System.Net bug test-run-core Test failures in .NET Core test runs
Milestone

Comments

@wfurt
Copy link
Member

wfurt commented Dec 16, 2022

https://helixre107v0xdeko0k025g8.blob.core.windows.net/dotnet-runtime-refs-heads-viktorhofer-outerlocb9d28b3bc0f4e38be/System.Net.WebClient.Tests/3/console.ad6886a8.log?helixlogtype=result

The UploadValues_Success test fails:

/private/tmp/helix/working/B9E809D7/w/A7880996/e /private/tmp/helix/working/B9E809D7/w/A7880996/e
  Discovering: System.Net.WebClient.Tests (method display = ClassAndMethod, method display options = None)
  Discovered:  System.Net.WebClient.Tests (found 14 of 46 test cases)
  Starting:    System.Net.WebClient.Tests (parallel test collections = on, max threads = 6)
    System.Net.Tests.TaskWebClientTest.UploadValues_Success(echoServer: http://corefx-net-http11.azurewebsites.net/Echo.ashx) [FAIL]
      Assert.Contains() Failure
      Not found: To+be%2c+or+not+to+be%2c+that+is+the+question%3aWhether+'tis+Nobler+in+the+mind+to+sufferThe+Slings+and+Arrows+of+outrageous+Fortune%2cOr+to+take+Arms+against+a+Sea+of+troubles%2cAnd+by+opposing+end+them%3a
      In value:  {"Method":"POST","Url":"/Echo.ashx","Headers": {
                     "Content-Length": "213",
                     "Content-Type": "application/x-www-form-urlencoded",
                     "Host": "corefx-net-http11.azurewebsites.net",
                     "Max-Forwards": "10",
                     "X-ARR-LOG-ID": "54501534-edad-4476-9600-256fcae425b1",
                     "CLIENT-IP": "131.107.1.155:9241",
                     "DISGUISED-HOST": "corefx-net-http11.azurewebsites.net",
                     "X-SITE-DEPLOYMENT-ID": "corefx-net-http11",
                     "WAS-DEFAULT-HOSTNAME": "corefx-net-http11.azurewebsites.net",
                     "X-Forwarded-For": "131.107.1.155:9241",
                     "X-Original-URL": "/Echo.ashx",
                     "X-WAWS-Unencoded-URL": "/Echo.ashx"
                   },
                   "Cookies": {},
                   "BodyContent": "Data=To+be%2C+or+not+to+be%2C+that+is+the+question%3AWhether+%27tis+Nobler+in+the+mind+to+sufferThe+Slings+and+Arrows+of+outrageous+Fortune%2COr+to+take+Arms+against+a+Sea+of+troubles%2CAnd+by+opposing+end+them%3A",
                   "BodyLength": 213,
                   "SecureConnection": false,
                   "ClientCertificatePresent": false,
                   "ClientCertificate": null
                 }
      Stack Trace:

It seems like the expected ' is escaped as %27.

cc: @MihaZupan

@wfurt wfurt added area-System.Net test-run-core Test failures in .NET Core test runs labels Dec 16, 2022
@wfurt wfurt added this to the 8.0.0 milestone Dec 16, 2022
@ghost
Copy link

ghost commented Dec 16, 2022

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

Issue Details

https://helixre107v0xdeko0k025g8.blob.core.windows.net/dotnet-runtime-refs-heads-viktorhofer-outerlocb9d28b3bc0f4e38be/System.Net.WebClient.Tests/3/console.ad6886a8.log?helixlogtype=result

/private/tmp/helix/working/B9E809D7/w/A7880996/e /private/tmp/helix/working/B9E809D7/w/A7880996/e
  Discovering: System.Net.WebClient.Tests (method display = ClassAndMethod, method display options = None)
  Discovered:  System.Net.WebClient.Tests (found 14 of 46 test cases)
  Starting:    System.Net.WebClient.Tests (parallel test collections = on, max threads = 6)
    System.Net.Tests.TaskWebClientTest.UploadValues_Success(echoServer: http://corefx-net-http11.azurewebsites.net/Echo.ashx) [FAIL]
      Assert.Contains() Failure
      Not found: To+be%2c+or+not+to+be%2c+that+is+the+question%3aWhether+'tis+Nobler+in+the+mind+to+sufferThe+Slings+and+Arrows+of+outrageous+Fortune%2cOr+to+take+Arms+against+a+Sea+of+troubles%2cAnd+by+opposing+end+them%3a
      In value:  {"Method":"POST","Url":"/Echo.ashx","Headers": {
                     "Content-Length": "213",
                     "Content-Type": "application/x-www-form-urlencoded",
                     "Host": "corefx-net-http11.azurewebsites.net",
                     "Max-Forwards": "10",
                     "X-ARR-LOG-ID": "54501534-edad-4476-9600-256fcae425b1",
                     "CLIENT-IP": "131.107.1.155:9241",
                     "DISGUISED-HOST": "corefx-net-http11.azurewebsites.net",
                     "X-SITE-DEPLOYMENT-ID": "corefx-net-http11",
                     "WAS-DEFAULT-HOSTNAME": "corefx-net-http11.azurewebsites.net",
                     "X-Forwarded-For": "131.107.1.155:9241",
                     "X-Original-URL": "/Echo.ashx",
                     "X-WAWS-Unencoded-URL": "/Echo.ashx"
                   },
                   "Cookies": {},
                   "BodyContent": "Data=To+be%2C+or+not+to+be%2C+that+is+the+question%3AWhether+%27tis+Nobler+in+the+mind+to+sufferThe+Slings+and+Arrows+of+outrageous+Fortune%2COr+to+take+Arms+against+a+Sea+of+troubles%2CAnd+by+opposing+end+them%3A",
                   "BodyLength": 213,
                   "SecureConnection": false,
                   "ClientCertificatePresent": false,
                   "ClientCertificate": null
                 }
      Stack Trace:

It seems like the expected ' is escaped as %27.

cc: @MihaZupan

Author: wfurt
Assignees: -
Labels:

area-System.Net, test-run-core

Milestone: 8.0.0

@MihaZupan
Copy link
Member

dotnet/corefx#18933 introduced an internal copy of the HttpUtility code for encoding the Uri.
For whatever reason, it wasn't an exact copy - it included ' in the "safe" set of characters.

#75896 removed this duplication and changed the behavior of ' again.

I'd be inclined to just update the test to use a new expected value.
It's not clear to me how this set of "safe" characters came to be - it doesn't make any sense to me when compared to Uri.

Compare the HttpUtility code

switch (ch)
{
case '-':
case '_':
case '.':
case '!':
case '*':
case '(':
case ')':
return true;
}

to the WebClient code in 7.0
switch (ch)
{
case '-':
case '_':
case '.':
case '!':
case '*':
case '\'':
case '(':
case ')':
return true;
}

@wfurt
Copy link
Member Author

wfurt commented Dec 16, 2022

I'm fine with that as long as we feel it will not break anybody. I just noticed failing tests as they run agains (tanks to @ViktorHofer)

@akoeplinger
Copy link
Member

@MihaZupan do you plan on updating the test soon or should we disable it in the meantime to get a clean run?

@akoeplinger akoeplinger added the disabled-test The test is disabled in source code against the issue label Dec 22, 2022
akoeplinger added a commit to akoeplinger/runtime that referenced this issue Dec 22, 2022
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Dec 26, 2022
@MihaZupan MihaZupan added the bug label Dec 26, 2022
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Dec 27, 2022
@MihaZupan MihaZupan removed the disabled-test The test is disabled in source code against the issue label Dec 27, 2022
@akoeplinger
Copy link
Member

@MihaZupan FYI the test needs to be reenabled in

[ActiveIssue("https://github.com/dotnet/runtime/issues/79731")]

@ghost ghost locked as resolved and limited conversation to collaborators Jan 26, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Net bug test-run-core Test failures in .NET Core test runs
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants