Skip to content

Conversation

ewillonermsft
Copy link
Contributor

Add additional HttpRequest properties to the System.Web.cs test stubs.

@ewillonermsft ewillonermsft requested a review from a team as a code owner September 25, 2025 01:25
@github-actions github-actions bot added the C# label Sep 25, 2025
Copy link
Contributor

@michaelnebel michaelnebel left a comment

Choose a reason for hiding this comment

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

Uh, thank you for doing this! 😄

public NameValueCollection Form => null;
public NameValueCollection Headers => null;
public NameValueCollection Params => null;
public string UserAgent(string s) => null;
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for bringing this up, I believe all of these properties need some adjustment (though worked on our side for our unittesting purposes). Please see the latest revision, I believe this better reflects the actual implementation as seen in ReferenceSource: HttpRequest.cs. If this is not correct please let me know and I can provide further changes.

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks good! And yes, the new signatures matches the signatures from the original implementation!

michaelnebel
michaelnebel previously approved these changes Sep 26, 2025
Copy link
Contributor

@michaelnebel michaelnebel left a comment

Choose a reason for hiding this comment

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

LGTM!

@michaelnebel michaelnebel self-requested a review September 26, 2025 07:46
@michaelnebel
Copy link
Contributor

It looks like the change to the stubs doesn't compile with all the tests.

@michaelnebel michaelnebel dismissed their stale review September 26, 2025 07:48

Failing test

@github github deleted a comment Sep 26, 2025
@github github deleted a comment Sep 26, 2025
@ewillonermsft
Copy link
Contributor Author

ewillonermsft commented Sep 26, 2025

@michaelnebel I don't have access to see which tests exactly in the "C# Language Tests Linux" pipeline is failing, I have started trying to kick them all off manually locally on my machine but I expect this process to take quite a while. I have found the "TaintedPath.qlref" test is failing which seems to be related to the .expected steps changing from this PR, but if you could provide me a full list so I can verify that would save me some time and hassle. Thank you!

@michaelnebel
Copy link
Contributor

@michaelnebel I don't have access to see which tests exactly in the "C# Language Tests Linux" pipeline is failing, I have started trying to kick them all off manually locally on my machine but I expect this process to take quite a while. I have found the "TaintedPath.qlref" test is failing which seems to be related to the .expected steps changing from this PR, but if you could provide me a full list so I can verify that would save me some time and hassle. Thank you!

Oh, that makes it a lot harder to debug.
It appears that one of the tests that are using the stub is failing as the code no longer compiles (unfortunately we have a mix of tests that purely uses stubs and some that uses a combination of stubs and assemblies).
If you cherry-pick this commit then it should fix the problem with the failing test (I am not allowed to push the changes to your branch).

Thank you for your persistence!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants