Skip to content
This repository has been archived by the owner on Dec 11, 2019. It is now read-only.

Disable new tab background for tests #9489

Merged
merged 1 commit into from
Jun 21, 2017
Merged

Disable new tab background for tests #9489

merged 1 commit into from
Jun 21, 2017

Conversation

cezaraugusto
Copy link
Contributor

Submitter Checklist:

  • Submitted a ticket for my issue if one did not already exist.
  • Used Github auto-closing keywords in the commit message.
  • Added/updated tests for this change (for new code or code which already has tests).
  • Ran git rebase -i to squash commits (if needed).
  • Tagged reviewers and labelled the pull request as needed.

Auditors: @bsclifton
Fix: #9240

Most tests were created while new tab was a blank page and/or before we had the option to show gradient background.

Test Plan:

  1. While running tests, new tab background should be shown as background gradient rather than the background image.
  2. No test regressions should happen

Reviewer Checklist:

Tests

  • Adequate test coverage exists to prevent regressions
  • Tests should be independent and work correctly when run individually or as a suite ref
  • New files have MPL2 license header

@cezaraugusto cezaraugusto added this to the 0.19.x (Nightly Channel) milestone Jun 15, 2017
@cezaraugusto cezaraugusto self-assigned this Jun 15, 2017
Copy link
Contributor

@NejcZdovc NejcZdovc left a comment

Choose a reason for hiding this comment

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

I would go other way around. So I think that non of the tests need background so I would disable it on global level, so if NODE_ENV test set this value to false or when you initialize test in lib/brave. This way tests are lighter and you can enable if needed.

Copy link
Contributor

@luixxiul luixxiul left a comment

Choose a reason for hiding this comment

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

Isn't documentation needed on docs/ to let people know that the images are disabled by default and they need to set something to run tests that include dashboard images?

Copy link
Contributor

@luixxiul luixxiul left a comment

Choose a reason for hiding this comment

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

++ on docs update :-)

@NejcZdovc
Copy link
Contributor

@cezaraugusto can you please squash commits

Copy link
Member

@bsclifton bsclifton left a comment

Choose a reason for hiding this comment

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

Great change; this will help save testing time! 😄

@bsclifton
Copy link
Member

@NejcZdovc can you please re-review (and merge if it looks OK 😄 )

Copy link
Contributor

@NejcZdovc NejcZdovc left a comment

Choose a reason for hiding this comment

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

LGTM ++

@NejcZdovc NejcZdovc merged commit 63ff18d into master Jun 21, 2017
@cezaraugusto cezaraugusto deleted the tests/9240 branch June 21, 2017 08:31
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants