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

Redirect tons of firefox logs to /dev/null #732

Merged
merged 2 commits into from May 9, 2018

Conversation

Projects
None yet
6 participants
@rosolko
Copy link
Collaborator

rosolko commented May 8, 2018

Proposed changes

Generally this PR addressed to #673
Linked with firefox bug that still open
Selenium bug - SeleniumHQ/selenium#2632
And geckodriver bug - mozilla/geckodriver#619

Checklist

  • Checkstyle and unit tests pass locally with my changes by running gradle check chrome htmlunit command
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)

@rosolko rosolko added this to the 4.11.4 milestone May 8, 2018

@rosolko rosolko self-assigned this May 8, 2018

@rosolko rosolko requested review from asolntsev and BorisOsipov May 8, 2018

@rosolko rosolko added the not sure label May 8, 2018

@rosolko rosolko changed the title Redirect tons of firefox spam logs to /dev/null Redirect tons of firefox spam to /dev/null May 8, 2018

@rosolko rosolko removed the not sure label May 8, 2018

@rosolko rosolko changed the title Redirect tons of firefox spam to /dev/null Redirect tons of firefox logs to /dev/null May 8, 2018

@@ -23,6 +23,7 @@ boolean supports() {

@Override
WebDriver create(final Proxy proxy) {
System.setProperty(FirefoxDriver.SystemProperty.BROWSER_LOGFILE, "/dev/null");

This comment has been minimized.

@BorisOsipov

BorisOsipov May 8, 2018

Member
System.setProperty(FirefoxDriver.SystemProperty.BROWSER_LOGFILE, System.getProperty(FirefoxDriver.SystemProperty.BROWSER_LOGFILE, "/dev/null"))

maybe ?

This comment has been minimized.

@rosolko

rosolko May 8, 2018

Collaborator

We need to decide - do we really want to provide way to configure this property.
If yes - we have to introduce new configuration field.
If not - just hide this with default value.

Because if use suggested code - it will be undocumented configuration.

This comment has been minimized.

@asolntsev

asolntsev May 9, 2018

Contributor

I vote for not adding new configuration field. Let's just use system property. Yes, it will be an "undocumented" feature, but almost nobody does really need to change it.

@coveralls

This comment has been minimized.

Copy link

coveralls commented May 8, 2018

Coverage Status

Coverage decreased (-0.1%) to 66.854% when pulling 06a5980 on rosolko:firefoxLogs into 3833a46 on codeborne:master.

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented May 8, 2018

Codecov Report

Merging #732 into master will decrease coverage by 0.15%.
The diff coverage is 0%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #732      +/-   ##
============================================
- Coverage     63.73%   63.58%   -0.16%     
+ Complexity      879      878       -1     
============================================
  Files           154      154              
  Lines          3025     3029       +4     
  Branches        298      298              
============================================
- Hits           1928     1926       -2     
- Misses          999     1004       +5     
- Partials         98       99       +1
Impacted Files Coverage Δ Complexity Δ
...selenide/webdriver/LegacyFirefoxDriverFactory.java 33.33% <0%> (-9.53%) 3 <0> (ø)
...borne/selenide/webdriver/FirefoxDriverFactory.java 83.33% <0%> (-4.91%) 10 <0> (ø)
...e/selenide/impl/WebDriverThreadLocalContainer.java 80.88% <0%> (-1.48%) 30% <0%> (-1%)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3833a46...06a5980. Read the comment docs.

@vinogradoff

This comment has been minimized.

Copy link
Member

vinogradoff commented May 8, 2018

The number of configurations is growing. Let's consider splitting into - basic configs / adavnced / expert configs?

@rosolko

This comment has been minimized.

Copy link
Collaborator

rosolko commented May 8, 2018

@vinogradoff Not sure about this.
From my point of view it will be not intuitive for users.

@asolntsev

This comment has been minimized.

Copy link
Contributor

asolntsev commented May 9, 2018

@rosolko @vinogradoff By the way, this PR fixes issue #673

@asolntsev
Copy link
Contributor

asolntsev left a comment

Good! Let's merge it.

@rosolko rosolko merged commit b6207b4 into selenide:master May 9, 2018

1 of 4 checks passed

codecov/patch 0% of diff hit (target 63.73%)
Details
codecov/project 63.58% (-0.16%) compared to 3833a46
Details
coverage/coveralls Coverage decreased (-0.1%) to 66.854%
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@rosolko rosolko deleted the rosolko:firefoxLogs branch May 9, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment