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

Refactor tests #1464

Merged
merged 6 commits into from
Jan 2, 2020
Merged

Refactor tests #1464

merged 6 commits into from
Jan 2, 2020

Conversation

stoically
Copy link
Member

@stoically stoically commented Jul 17, 2019

Here's a test change only - updated with the latest dependencies - which was previously in #1190, but since it doesn't touch production code, I think it's good to merge into master.

@stoically stoically changed the title Refactor tests, Add container feature test Refactor tests Jul 17, 2019
@stoically stoically mentioned this pull request Jul 17, 2019
@maxxcrawford maxxcrawford self-requested a review October 17, 2019 19:05
@maxxcrawford
Copy link
Collaborator

@stoically I'm not able to run this patch with all passing tests. Is there something wrong on my end or is that a known issue with this patch?

@stoically
Copy link
Member Author

@maxxcrawford You're right, it seems something introduced a regression since I've opened the PR (travis was green back then). Reopening to trigger a red build and looking into it when I get the chance.

@stoically stoically closed this Oct 18, 2019
@stoically stoically reopened this Oct 18, 2019
@stoically
Copy link
Member Author

Interesting. So travis is green again, but locally creating a new container should create it in the browser as well fails for me. Odd.

@stoically
Copy link
Member Author

stoically commented Dec 6, 2019

@maxxcrawford rebased and updated to latest webextensions-jsdom, which depends on latest webextensions-api-fake, and now works for me locally as expected. Could you give it another look? Thanks!

Copy link
Collaborator

@maxxcrawford maxxcrawford left a comment

Choose a reason for hiding this comment

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

Thanks for working on these test rewrites. I'm trying to get a better sense of this testing suite (and the project as a whole). Could you break this PR into multiple commits for better readability? Specifically, separating the coverage, dependencies updates and each test (where applicable). Still one PR, but enough different commits to step through the refactor easier.

The good news is that Mozilla has bandwidth (and an intern!) to put towards this project over the next three months.

@stoically stoically force-pushed the refactor-tests branch 2 times, most recently from 0247ee9 to 1932181 Compare December 18, 2019 08:33
@stoically
Copy link
Member Author

Separated the changes into multiple commits as requested, hope this helps with readability. As an aside: Facebook Container uses the dependency webextensions-jsdom as well.

Looking forward to see progress in MAC!

@stoically stoically force-pushed the refactor-tests branch 2 times, most recently from 71a1e35 to b10332e Compare December 18, 2019 08:39
Copy link
Collaborator

@maxxcrawford maxxcrawford left a comment

Choose a reason for hiding this comment

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

Thanks for all the work on this refactor!

@jonathanKingston jonathanKingston merged commit dc9e8f6 into mozilla:master Jan 2, 2020
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