Skip to content
This repository was archived by the owner on Jan 20, 2022. It is now read-only.

Bug 1145112 - Convert all usages of wait_for_condition to Wait().until(). r=whimboo#144

Closed
sr-murthy wants to merge 27 commits intomozilla:masterfrom
sr-murthy:bugfix-1145112
Closed

Bug 1145112 - Convert all usages of wait_for_condition to Wait().until(). r=whimboo#144
sr-murthy wants to merge 27 commits intomozilla:masterfrom
sr-murthy:bugfix-1145112

Conversation

@sr-murthy
Copy link
Copy Markdown

Replaced all instances of wait_for_condition() with Wait().until()

@sr-murthy sr-murthy changed the title Bugfix 1145112 Bug 1145112 Mar 28, 2015
@whimboo whimboo changed the title Bug 1145112 Bug 1145112 - Convert all usages of wait_for_condition to Wait().until(). r=whimboo May 6, 2015
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

So this change looks fine, but have you run the tests locally yourself? At least this test will fail because Wait has not been imported.

@whimboo
Copy link
Copy Markdown
Contributor

whimboo commented May 8, 2015

Beside the above comments please also check the reported Travis failures and get them fixed. Travis CI has to pass before we can merge anything.

@sr-murthy
Copy link
Copy Markdown
Author

OK, will do these soon.

@sr-murthy
Copy link
Copy Markdown
Author

I've made the requested changes, but I cannot acces the Travis CI build. Also the build did not trigger in the normal way.

@whimboo
Copy link
Copy Markdown
Contributor

whimboo commented May 13, 2015

Thanks for the update! So the test via Travis has not been triggered because there is a merge conflict. You will have to rebase your development branch against the latest changes on upstream/master first. Once this is done Travis will pick up the latest state of the branch and run the tests.

@sr-murthy
Copy link
Copy Markdown
Author

I thought I had fixed the rebase conflicts, since I was able to finish the rebase. Now the Travis build fails again.

@sr-murthy
Copy link
Copy Markdown
Author

Looking at the build failure the errors seem related to PEP8 checks, not my code changes. I am going through the PEP8 errors one by one, and will push again after I have addressed all of them.

@whimboo
Copy link
Copy Markdown
Contributor

whimboo commented May 18, 2015

It looks like that you got the merge done now. That's great. But as you can see Travis is still not happy due to Pep8 problems. You will have to go through each of the lines and fix what the test output shows. If you have specific questions you can always join us on IRC and we can have a direct conversation about a specific problem.

@sr-murthy
Copy link
Copy Markdown
Author

OK, I'll turn off the Travis CI build option for the fork and then push again.

@sr-murthy
Copy link
Copy Markdown
Author

Sorry for the delay, but I in a week I will fix the PEP8 errors and update the pull request.

@sr-murthy
Copy link
Copy Markdown
Author

RIght, finally I have been able to find the time to resolve the PEP8 errors. I note that your advice to combine two Wait().until() statements into one conflicts with the PEP8 line length limit of 99 characters per line. This is the error I get for line 52 in functional/locationbar/test_access_locationbar.py when I run the pep8 tool locally:

$ pep8 --max-line-length=99 --exclude=client firefox_ui_tests/functional/locationbar/test_access_locationbar.py
firefox_ui_tests/functional/locationbar/test_access_locationbar.py:51:100: E501 line too long (136 > 99 characters)

@sr-murthy sr-murthy closed this Jun 13, 2015
@sr-murthy sr-murthy reopened this Jun 13, 2015
@sr-murthy
Copy link
Copy Markdown
Author

I've resolved the PEP8 errors (and testing this locally using the pep8 tool) but again rebasing the development branch against my updated local master causes problems. Specifically, what I do is do a git fetch of the upstream master to the local master then merge, switch to the dev branch and do a git rebase -i master. I choose only the commits I want, deleting those which are unnecessary, and squashing all the most recent relevant commits to the first one, but I get the following message:

git rebase -i master
error: could not apply 1a712f6... Replaced all instances of wait_for_condition() with Wait().until()

When you have resolved this problem, run "git rebase --continue".
If you prefer to skip this patch, run "git rebase --skip" instead.
To check out the original branch and stop rebasing, run "git rebase --abort".
Could not apply 1a712f6cb827e631d906138ac81d6fbfd10e0349... Replaced all instances of wait_for_condition() with Wait().until()

@sr-murthy
Copy link
Copy Markdown
Author

I'm going to close this, and create a new local dev branch from which I will create a new pull request.

@sr-murthy sr-murthy closed this Jun 14, 2015
@sr-murthy sr-murthy deleted the bugfix-1145112 branch June 26, 2015 00:22
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants