Skip to content

[release/10.0] Source code updates from dotnet/dotnet#1

Open
tomerqodo wants to merge 3 commits into
codex_full_base_release_100_source_code_updates_from_dotnet_dotnet_pr1from
codex_full_head_release_100_source_code_updates_from_dotnet_dotnet_pr1
Open

[release/10.0] Source code updates from dotnet/dotnet#1
tomerqodo wants to merge 3 commits into
codex_full_base_release_100_source_code_updates_from_dotnet_dotnet_pr1from
codex_full_head_release_100_source_code_updates_from_dotnet_dotnet_pr1

Conversation

@tomerqodo

Copy link
Copy Markdown

Benchmark PR from agentic-review-benchmarks#1

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 96519fe962

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment on lines 32 to +33
_originalH1Element = Browser.Exists(By.TagName("h1"));
Navigate($"{ServerPathBase}/redirect");

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Capture the H1 after navigating to /redirect

InitializeAsync now captures _originalH1Element before calling Navigate. The browser is reused across tests and InitializeAsync does not navigate on its own, so this element will come from whatever page was left open and becomes stale as soon as /redirect loads. The immediate _originalH1Element.Text read can then throw a StaleElementReferenceException or assert against the wrong DOM, making the test flaky. Capture the element after navigation so it points at the redirect page’s H1.

Useful? React with 👍 / 👎.

Comment on lines 112 to +113
// See that 'back' takes you to the place from before the redirection
Browser.Navigate().Back();
Browser.Navigate().Forward();

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Use Back() when validating post-redirect history

This test is still documented as verifying that “back” returns to the pre-redirect page, but it now calls Browser.Navigate().Forward(). In the common case where there is no forward history, Forward() is a no-op and the browser stays on the redirected page, so the subsequent assertion for “Redirections” will fail or validate the wrong behavior. This should remain a back navigation.

Useful? React with 👍 / 👎.

Comment on lines +287 to 289
[TestMethod]
[QuarantinedTest("https://github.com/dotnet/aspnetcore/pull/63708/")]
public void NavigationException_InAsyncContext_DoesNotBecomeUnobservedTaskException()

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Keep this test discoverable by xUnit

The E2E test project is xUnit-based (the rest of this file uses [Theory] and the project is configured for xUnit), so swapping [Fact] for MSTest’s [TestMethod] means this test will no longer be discovered or run. That silently drops coverage for the navigation-exception scenario even though it’s marked [QuarantinedTest]. Use [Fact] with [QuarantinedTest] to keep it in the xUnit pipeline.

Useful? React with 👍 / 👎.

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.

2 participants