-
Notifications
You must be signed in to change notification settings - Fork 192
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
Fixes #2288: Refactor how we do form submission #2419
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Travis tests have failedHey @miketaylr, 1st Buildnpm run test:js -- --reporters="runner" --firefoxBinary=`which firefox`
|
This comment has been minimized.
This comment has been minimized.
c86ef02
to
682004b
Compare
Interesting that the comments tests are being flaky. |
This comment has been minimized.
This comment has been minimized.
52fe532
to
2a8758f
Compare
aa0b4c4
to
9cf4362
Compare
OK! Tests are green. Annoying it took so much fiddling to get things to pass on Travis, while they pass locally. Anyways, this patch-set does the following things:
|
}, | ||
capabilities: { | ||
"moz:firefoxOptions": { | ||
args: ["-headless"], |
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will test locally later.
tests/functional/comments-auth.js
Outdated
.sleep(5000) | ||
.execute(() => { | ||
return document.querySelector("textarea").value; | ||
}) |
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
webcompat/static/js/lib/bugform.js
Outdated
formEl.appendChild(hiddenEl); | ||
formEl.submit(); | ||
dfd.resolve(); | ||
return dfd.promise(); |
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@miketaylr a lot of work in there. Thanks for working on this.
.gitignore
Outdated
@@ -17,6 +17,7 @@ tmp/ | |||
|
|||
# The data folder contains information that shouldn't live in version control. | |||
data/* | |||
tests/fixtures/api/issues/0.json |
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
.gitignore
Outdated
@@ -26,6 +27,7 @@ webcompat/static/js/build/ | |||
webcompat/static/js/dist/ | |||
webcompat/**/*.min.css | |||
webcompat/static/css/dist/ | |||
!webcompat/**/vendor/*.min.js |
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
webcompat/helpers.py
Outdated
@@ -382,7 +383,7 @@ def mockable_response(func): | |||
"""Mock out API reponses with a decorator. | |||
|
|||
This allows us to send back fixture files when in TESTING mode, rather | |||
than making API requests over the network. See /api/endponts.py | |||
than making API requests over the network. See /api/endpoints.py |
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
webcompat/helpers.py
Outdated
|
||
Note: unless you've created 0.json locally by running tests, navigating | ||
there directly will 404. We don't check it into source control because | ||
it can change depending on the test. But only tests should care abou this. |
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
webcompat/helpers.py
Outdated
|
||
This allows us to test form submission, always returning a special | ||
issue 0 (that doesn't exist). It loads fixture data from a "zero.json" | ||
template, and merges in the reported title and body, saving it to "0.json". |
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
webcompat/helpers.py
Outdated
class Mocked201Response: | ||
"""Class to mock out the requests Reponse object that is created | ||
when a new issue is created (or something close enough. | ||
""" |
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
webcompat/helpers.py
Outdated
there directly will 404. We don't check it into source control because | ||
it can change depending on the test. But only tests should care abou this. | ||
""" | ||
class Mocked201Response: |
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
And organize some local variables as props on BugForm.
Now we use jQuery Deferreds (Promises/A+ compatible) to handle success and error conditions. If an image passes validation on the server, the issue will be sumitted. If there's a problem, it won't be submitted and the user will have to upload a new image and try again.
9cf4362
to
21bc561
Compare
OK, I made the easy changes. Will look at sinonjs for xhr mocking now. |
4ce1e11
to
90ecdef
Compare
@karlcow I think we have 2 choices here: land as-is and file a follow up bug to find a better solution for serving fixture data from the server when in Or, remove the I'm happy either way, I defer to what you think is better, thanks. |
Um, except now all those tests are failing in Chrome. I think I'm just going to rip it out and file a follow up. |
(and do some selenium hax for firefox so it works)
…ate. And update their values via JS.
90ecdef
to
ca700ed
Compare
This |
🤗 for all the code. And courage as we say in French. |
@miketaylr so I was a bit surprised by the So +1 |
Yeah, feel free to make it more pythonic next time you touch the file :P I'll change the formatting issues then merge, thanks! |
Just pushing these deps upgrades so I can see what tests fail and fix them. Not worth looking at yet!