Skip to content

Fixes #1711. Ensure change event fires when same image is selected.#1718

Merged
miketaylr merged 3 commits into
masterfrom
issues/1711/1
Aug 21, 2017
Merged

Fixes #1711. Ensure change event fires when same image is selected.#1718
miketaylr merged 3 commits into
masterfrom
issues/1711/1

Conversation

@miketaylr

Copy link
Copy Markdown
Member

@miketaylr miketaylr requested a review from zoepage August 16, 2017 03:01

@zoepage zoepage left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It's a nice solution! Thanks!

One test is failing:

× firefox on any platform - Comments (auth) - Add a screenshot to a comment
AssertionError: The image was correctly uploaded and its URL was copied to the comment text.: expected '' to include '[![Screenshot Description](http://localhost:5000/uploads/'
  at Command.<anonymous>  <tests/functional/comments-auth.js:149:20>
  at Test.Add a screenshot to a comment [as test]  <tests/functional/comments-auth.js:148:12>

Probably just a minor thing :)

@miketaylr

Copy link
Copy Markdown
Member Author

One test is failing:

@zoepage Did that fail for you locally? In Chrome or Firefox? Interesting that Travis passed all tests.

@miketaylr

Copy link
Copy Markdown
Member Author

All tests are passing for me locally as well, in Chrome and Firefox.

0/81 tests failed (3 skipped)

@zoepage

zoepage commented Aug 16, 2017

Copy link
Copy Markdown
Member

This failed locally. Ran it twice. Same result. 🤔
The branch is clean and up to date.
screen shot 2017-08-16 at 9 27 45 pm

Which npm / Node version do you have installed?
-> node v6.9.2
-> npm v3.10.9

@karlcow

karlcow commented Aug 17, 2017

Copy link
Copy Markdown
Member
workon webcompatcom
git checkout master
git fetch webcompat
git merge webcompat/master
node -v; npm -v
# v6.10.2
# 3.10.10
git checkout -b issues/1711/1 webcompat/issues/1711/1
# Branch issues/1711/1 set up to track remote branch issues/1711/1 from webcompat.
# Switched to a new branch 'issues/1711/1'
python run.py -t
# Starting server in ~*TEST MODE*~
# * Running on http://localhost:5000/ (Press CTRL+C to quit)
# * Restarting with stat
# Starting server in ~*TEST MODE*~

Another window and Firefox 55.0.2

workon webcompatcom


java -version
# java version "1.8.0_111"
# Java(TM) SE Runtime Environment (build 1.8.0_111-b14)
# Java HotSpot(TM) 64-Bit Server VM (build 25.111-b14, mixed mode)
node_modules/.bin/intern-runner config=tests/intern

and… lol?

capture d ecran 2017-08-17 a 10 52 24

@miketaylr

Copy link
Copy Markdown
Member Author

What in the world.

@miketaylr

Copy link
Copy Markdown
Member Author

If tests are passing on Travis, I'm not sure what to do about failures on y'alls local machines. We trust Travis as the main source of truth for tests...

@miketaylr

Copy link
Copy Markdown
Member Author

@zoepage can you change the sleep here to like, 5000 or something and re-run this test? I wonder if we're running into timeouts based on different machines.

https://github.com/webcompat/webcompat.com/blob/master/tests/functional/comments-auth.js#L144

(either way, this test failure shouldn't block, IMO -- these changes are unrelated to the codepaths this test is testing -- issue comment screenshots have nothing to do with change events)

@zoepage

zoepage commented Aug 21, 2017

Copy link
Copy Markdown
Member

If tests are passing on Travis, I'm not sure what to do about failures on y'alls local machines. We trust Travis as the main source of truth for tests...

Never trust the humans, I agree.
Rerunning the tests with a 5000 timeout right now, but if you want to merge.. go for it.

@zoepage

zoepage commented Aug 21, 2017

Copy link
Copy Markdown
Member

FYI, tests are passing 🎉

@miketaylr

Copy link
Copy Markdown
Member Author

FYI, tests are passing 🎉

thanks!

@karlcow

karlcow commented Aug 21, 2017

Copy link
Copy Markdown
Member

Unrelated to this PR, but I wonder if @andreastt would be interested by the results in comment #1718 (comment) aka Chrome passing all the tests and Firefox randomly failing them.

@zoepage zoepage deleted the issues/1711/1 branch August 22, 2017 08:30
@andreastt

Copy link
Copy Markdown

@karlcow With the limited amount of information that is available here, it is impossible to say anything definite about them. I am assuming you are expecting parity in results and for the Firefox tests to pass.

  • The client may not support the WebDriver specification
  • The tests may be utilising the API in a way that is not backwards-compatible with Selenium
  • Firefox may not yet support a particular feature that is used
  • Some of the failures could be systemic, i.e. an underlying problem with infrastructure could be blamed

Without a concrete idea what you are doing and what you are expecting the result to be, and a geckodriver trace-level log (see the README on how to emit one), I can only make educated guesses what might be causing Firefox to “randomly fail them”.

@miketaylr

Copy link
Copy Markdown
Member Author

(note: travis is now failing on that same test that @zoepage mentioned, lol -- i'll push a follow up fix to master)

@zoepage

zoepage commented Aug 23, 2017

Copy link
Copy Markdown
Member

@miketaylr is the reason for the failing test the timeout for the response?

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.

4 participants