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

fixes failing tab transfer test #9304

Merged
merged 1 commit into from
Jun 7, 2017
Merged

fixes failing tab transfer test #9304

merged 1 commit into from
Jun 7, 2017

Conversation

bridiver
Copy link
Collaborator

@bridiver bridiver commented Jun 7, 2017

https://travis-ci.org/brave/browser-laptop/jobs/239787005#L3218

Strictly speaking this was only a test issue, but I did a small refactoring to make the functionality more consistent and easier to test

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.

Test Plan:

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

@bridiver bridiver added this to the 0.18.x milestone Jun 7, 2017
@bridiver bridiver self-assigned this Jun 7, 2017
@bridiver bridiver requested review from bbondy and NejcZdovc June 7, 2017 16:15
@NejcZdovc
Copy link
Contributor

can you please check if any of tests that are failing now are related to your change

  1) pinnedTabs gets pins from external windows when pinning after creating:
     WaitUntilTimeoutError: Promise was rejected with the following reason: timeout
      at elements(".pinnedTabs [data-test-id="tab"]") - brave.js:428:21

  2) tab tests tab transfer can move into an existing window:
     undefined: Cannot read property 'tabId' of undefined
      at execute(<Function>) - brave.js:526:19

  3) tab tests tab transfer can detach the last tab into an existing window:
     Promise was rejected with the following reason: timeout
  Error

@bridiver
Copy link
Collaborator Author

bridiver commented Jun 7, 2017

this fixes #3 and was passing for me locally. I'll check on the others

@NejcZdovc
Copy link
Contributor

NejcZdovc commented Jun 7, 2017

In PR you said that this should fix tab tests tab transfer can detach into new windows, but number 3 is tab tests tab transfer can detach the last tab into an existing window it's a different test.

All this 3 tests are failing for me locally.

@bridiver
Copy link
Collaborator Author

bridiver commented Jun 7, 2017

yep, read it wrong. I'll check all 3

@bridiver
Copy link
Collaborator Author

bridiver commented Jun 7, 2017

looks like a problem with the test themselves, not the code changes. I'll update them

@NejcZdovc
Copy link
Contributor

thank you very much

@bridiver bridiver force-pushed the tab-transfer-test branch from 51e632d to 57a82d6 Compare June 7, 2017 19:13
@bridiver
Copy link
Collaborator Author

bridiver commented Jun 7, 2017

the pinned tab test passes for me and the others are updated

@NejcZdovc NejcZdovc merged commit a3b740c into master Jun 7, 2017
@bbondy bbondy deleted the tab-transfer-test branch June 21, 2017 20:10
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.

2 participants