1219678 - Implement *test_number_keyboard.py* as an integration test in JavaScript

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Keyboard, defect)

Description

[William Hsu [:whsu]]

https://github.com/mozilla-b2g/gaia/tree/master/tests/python/gaia-ui-tests/gaiatest/tests/functional/keyboard/test_number_keyboard.py

Comment 1

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Ray will take a look, thanks!

Comment 2

[Ray Lin[:ralin]]

since number input type is not covered by gij now, I'll implement this part(as link shown) to a new suite in gij.

https://github.com/mozilla-b2g/gaia/blob/e68d693cb55fb5d8946498eb2bdb63f55116d38e/tests/python/gaia-ui-tests/gaiatest/tests/functional/keyboard/test_number_keyboard.py#L20-L30

Comment 3

[GitHub Autolander Bot]

Attachment: [gaia] raylin:1219678-test_number_keyboad-gij > mozilla-b2g:master

Comment 4

[GitHub Autolander Bot]

Attachment: [gaia] raylin:keyboard-tests-gip-to-gij > mozilla-b2g:master

Comment 5

[GitHub Autolander Bot]

Attachment: [gaia] raylin:1219678-test-number-keyboard-gij > mozilla-b2g:master

Comment 6

[Ray Lin[:ralin]]

Comment on attachment 8684798 [details] [review]
[gaia] raylin:1219678-test-number-keyboard-gij > mozilla-b2g:master

1. this test is now covered by the following code segment in gij.

https://github.com/raylin/gaia/blob/66ac0dc10073bfa300f9cfb568b936bf3a86e89e/apps/keyboard/test/marionette/number_keyboard_test.js#L50-L90

2. moved input_test.js to text_keyboard_test.js

Comment 7

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8684798 [details] [review]
[gaia] raylin:1219678-test-number-keyboard-gij > mozilla-b2g:master

Nice work! I've left a few comments on GitHub.

Please make sure your work is not just about translate Python code to JavaScript, but also read and evaluate the test cases to see if the assertions and operations are actually valid. We don't want to add false positive tests :).

Comment 8

[GitHub Autolander Bot]

Attachment: [gaia] raylin:1219678-test-number-keyboard-gij-r1 > mozilla-b2g:master

Comment 9

[Ray Lin[:ralin]]

Comment on attachment 8685304 [details] [review]
[gaia] raylin:1219678-test-number-keyboard-gij-r1 > mozilla-b2g:master

try to refine prior commit and follow the principles in comments.

Comment 10

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8685304 [details] [review]
[gaia] raylin:1219678-test-number-keyboard-gij-r1 > mozilla-b2g:master

This is a very nice re-organization of the tests. Thank you for doing this. 

Preferably if a set of re-arrangement of code is needed before actually implementing the test, these underlining work should be split into another "Part I" commit (you can land multiple commits in a pull request as long as they all begin with a Bug #).

If the re-arrangement is larger than desired, you could also file a dependent bug and work/review/land that patch in that bug too.

Comment 11

[GitHub Autolander Bot]

Attachment: [gaia] raylin:1219678-test-number-keyboard-gij-r2 > mozilla-b2g:master

Comment 12

[Ray Lin[:ralin]]

Comment on attachment 8685924 [details] [review]
[gaia] raylin:1219678-test-number-keyboard-gij-r2 > mozilla-b2g:master

Thanks a lot for your kind information. It does meet my questions and help me to improve my workflow. If possible, I'm willing to polish gij in Keyboard app after these five bugs.

I updated the non-descriptive comment.
Thanks.

Comment 13

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8685924 [details] [review]
[gaia] raylin:1219678-test-number-keyboard-gij-r2 > mozilla-b2g:master

No need to seek r+ once you already got one.

Also, you don't really need to file new pull requests for every review unless you explicitly want to record-keeping the changes.

Make sure you close the pull requests that shouldn't be merged.

Comment 14

[Ray Lin[:ralin]]

Yes, I see. I'll make it better next time.
Thanks.

Comment 15

[Tim Guan-tin Chien [:timdream] (please needinfo)]

master: https://github.com/mozilla-b2g/gaia/commit/ddceb5a6ec7b8664ffd0d9147c213838d52e08d1