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

Set default window sizes #128

Merged
merged 4 commits into from
Oct 17, 2019
Merged

Conversation

dbjorge
Copy link
Contributor

@dbjorge dbjorge commented Oct 16, 2019

Description of changes

Per suggestion from @manishsat , this PR has the samples set a default window size explicitly, to avoid issues related to the color contrast test's "scroll into viewport" behavior in the default window sizes used by the browsers' headless modes (eg, dequelabs/axe-core#1730)

I verified the syntax of the window size parameters locally by temporarily disabling the headless options and manually verifying the created window sizes.

Pull request checklist

  • [n/a] If this PR addresses an existing issue, it is linked: Fixes #0000
  • New sample content is commented at a similar verbosity as existing content
  • All updated/modified sample code builds and runs in at least one PR/CI build
  • PR checks for builds named [failing example] ... fail as expected

@dbjorge dbjorge requested a review from a team as a code owner October 16, 2019 21:13
@dbjorge dbjorge mentioned this pull request Oct 16, 2019
3 tasks
dbjorge added a commit that referenced this pull request Oct 16, 2019
#### Description of changes

During local testing of #128, I noticed that the .NET Core + Windows + Firefox environment combination was excessively slow compared to all other combinations (taking >2min per test run instead of <10s).

This turns out to be caused by mozilla/geckodriver#1496 / https://bugzilla.mozilla.org/show_bug.cgi?id=1525659 - if I'm understanding the bugs correctly, geckodriver listens for selenium commands on IPv4 only by default and selenium tries to connect via "http://localhost" rather than "http://127.0.0.1", but .NET Core's standard HttpClient behavior under Windows ends up attempting to connect to "localhost" URLs via IPv6 for a second before timing out and retrying on IPv4, so in practice this adds a second of overhead to every API call between Selenium and geckodriver.

This change works around it by forcing both of them to use a `::1` to communicate. The linked bug suggested that using `localhost` or `127.0.0.1` should have also worked around the issue, but it didn't seem to help when I tried it. Using `::1` in all cases prevented it from working in our linux build agents, so I ended up having to do a platform check.

Before:
```
> dotnet test --filter TestCategory!=IntentionallyFailsAsAnExample
Test run for Q:\repos\axe-pipelines-samples\csharp-selenium-webdriver-sample\bin\Debug\netcoreapp2.2\CSharpSeleniumWebdriverSample.dll(.NETCoreApp,Version=v2.2)
Microsoft (R) Test Execution Command Line Tool Version 16.0.1
Copyright (c) Microsoft Corporation.  All rights reserved.
Starting test execution, please wait...
Total tests: 3. Passed: 3. Failed: 0. Skipped: 0.
Test Run Successful.
Test execution time: 2.3717 Minutes
Command took 2:23.929.
```

After:
```
> dotnet test --filter TestCategory!=IntentionallyFailsAsAnExample
Test run for Q:\repos\axe-pipelines-samples\csharp-selenium-webdriver-sample\bin\Debug\netcoreapp2.2\CSharpSeleniumWebdriverSample.dll(.NETCoreApp,Version=v2.2)
Copyright (c) Microsoft Corporation.  All rights reserved.
Starting test execution, please wait...
Total tests: 3. Passed: 3. Failed: 0. Skipped: 0.
Test Run Successful.
Test execution time: 5.4795 Seconds
Command took 7.2035856 seconds.
```

#### Pull request checklist

- [n/a] If this PR addresses an existing issue, it is linked: Fixes #0000
- [x] New sample content is commented at a similar verbosity as existing content
- [x] All updated/modified sample code builds and runs in at least one PR/CI build
- [x] PR checks for builds named `[failing example] ...` fail as expected
@dbjorge dbjorge merged commit 44b0ff3 into microsoft:master Oct 17, 2019
@dbjorge dbjorge deleted the set-default-window-size branch October 17, 2019 22:47
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