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

Adding assignment feature tests #1107

Merged
merged 2 commits into from
Feb 9, 2018

Conversation

stoically
Copy link
Member

@stoically stoically commented Jan 31, 2018

Hi,

since jkt signaled on IRC that adding some tests to the codebase is OK, here's a proposal that loads popup.html and background/index.html with jsdom and executes a basic assignment feature test:

If you find this useful I could add more feature tests based on this. Of course I'm open to feedback and willing to change stuff - just let me know what you think.

Libs used: jsdom, mocha, sinon, chai

Bumped travis nodejs version to lts/* (latest LTS Node.js release) to have ES6 support in the tests

Best regards

@groovecoder
Copy link
Member

Whoa, this is a cool PR. @jonathanKingston - do you have time to check this out this week?

stoically added a commit to stoically/multi-account-containers that referenced this pull request Jan 31, 2018
@stoically
Copy link
Member Author

Glad you like it. Refactored a bit and wired popup.browser.runtime.sendMessage with background.browser.runtime.onMessage. That way the click in the popup automatically triggers the background which makes it imho a more reliable feature test.

stoically added a commit to stoically/multi-account-containers that referenced this pull request Jan 31, 2018
stoically added a commit to stoically/multi-account-containers that referenced this pull request Jan 31, 2018
stoically added a commit to stoically/multi-account-containers that referenced this pull request Jan 31, 2018
stoically added a commit to stoically/multi-account-containers that referenced this pull request Jan 31, 2018
stoically added a commit to stoically/multi-account-containers that referenced this pull request Feb 1, 2018
stoically added a commit to stoically/multi-account-containers that referenced this pull request Feb 2, 2018
stoically added a commit to stoically/multi-account-containers that referenced this pull request Feb 2, 2018
@stoically stoically changed the title Adding feature tests Adding assignment feature tests Feb 2, 2018
stoically added a commit to stoically/multi-account-containers that referenced this pull request Feb 2, 2018
stoically added a commit to stoically/multi-account-containers that referenced this pull request Feb 2, 2018
stoically added a commit to stoically/multi-account-containers that referenced this pull request Feb 2, 2018
@stoically
Copy link
Member Author

Finished the refactorings that I initially mentioned

  • Rephrase and refactor the feature test to be less technical and more black-box-ish
  • Don't check for browser.runtime.sendMessage and browser.storage.local.set but instead yield onBeforeRequest and check if MAC reopens the tab correctly
  • Move "popup clicking" and "opening of tabs" behind helper functions - would make writing future feature tests easier

stoically added a commit to stoically/multi-account-containers that referenced this pull request Feb 2, 2018
@stoically
Copy link
Member Author

stoically commented Feb 9, 2018

Tests currently fail because I've added one for #1114 - so if that should get merged, the tests would go green again. I can of course remove the test from this PR again, since it isn't really in the scope and in reality belongs to #1114. Just thought it's easier to see the test fail this way.

Copy link
Contributor

@jonathanKingston jonathanKingston left a comment

Choose a reason for hiding this comment

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

LGTM we should test in a release with the other patch mentioned though.

@jonathanKingston jonathanKingston changed the base branch from master to testing-6.0.0 February 9, 2018 17:59
@jonathanKingston jonathanKingston merged commit 40426ca into mozilla:testing-6.0.0 Feb 9, 2018
@stoically stoically deleted the tests branch February 10, 2018 00:03
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.

3 participants