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

Use wptserve for unit tests (#368) #369

Merged
merged 2 commits into from
Feb 9, 2016
Merged

Conversation

whimboo
Copy link
Contributor

@whimboo whimboo commented Feb 9, 2016

This PR will fix issue #368. @mjzffr can you please review it? Thanks

@@ -89,5 +89,5 @@ def handle_data(self, data):
if not self.active_url:
return

if self.active_url == data:
if data in self.active_url:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We need this change because the actual data can contain a final slash or not, while active_url always has it.

Copy link

Choose a reason for hiding this comment

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

Wouldn't it be safer to strip the final slash and then compare? data in self.active_url is a very generous condition.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let me check if that would work.

@mjzffr
Copy link

mjzffr commented Feb 9, 2016

One question, otherwise looks good.
Ideally, I would use a separate commit for the switch from mozlog to logging.

@whimboo
Copy link
Contributor Author

whimboo commented Feb 9, 2016

@mjzffr I pushed another commit with some clean-up + a new test for names with spaces. Wanted to be sure that wptserve handles that correctly. Let me know if you like that now.

Regarding the mozlog changes I can redo the commits after I got a r+. Sadly I missed those changes with my recent landing for issue #360. I could re-label it appropriately.

@mjzffr
Copy link

mjzffr commented Feb 9, 2016

@whimboo r+

@whimboo
Copy link
Contributor Author

whimboo commented Feb 9, 2016

I separated the commits for the wptserve and logging changes. Travis is running now. If results are passing I will get it landed.

@whimboo whimboo merged commit a46d719 into mozilla:master Feb 9, 2016
@whimboo whimboo deleted the wptserve branch February 9, 2016 21:15
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.

None yet

2 participants