{"action":"created","issue":{"url":"https://api.github.com/repos/vaadin/flow/issues/3029","repository_url":"https://api.github.com/repos/vaadin/flow","labels_url":"https://api.github.com/repos/vaadin/flow/issues/3029/labels{/name}","comments_url":"https://api.github.com/repos/vaadin/flow/issues/3029/comments","events_url":"https://api.github.com/repos/vaadin/flow/issues/3029/events","html_url":"https://github.com/vaadin/flow/pull/3029","id":276376139,"number":3029,"title":"Add tests for IronList component","user":{"login":"gilberto-torrezan","id":7464735,"avatar_url":"https://avatars2.githubusercontent.com/u/7464735?v=4","gravatar_id":"","url":"https://api.github.com/users/gilberto-torrezan","html_url":"https://github.com/gilberto-torrezan","followers_url":"https://api.github.com/users/gilberto-torrezan/followers","following_url":"https://api.github.com/users/gilberto-torrezan/following{/other_user}","gists_url":"https://api.github.com/users/gilberto-torrezan/gists{/gist_id}","starred_url":"https://api.github.com/users/gilberto-torrezan/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/gilberto-torrezan/subscriptions","organizations_url":"https://api.github.com/users/gilberto-torrezan/orgs","repos_url":"https://api.github.com/users/gilberto-torrezan/repos","events_url":"https://api.github.com/users/gilberto-torrezan/events{/privacy}","received_events_url":"https://api.github.com/users/gilberto-torrezan/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":5,"created_at":"2017-11-23T13:32:07Z","updated_at":"2017-11-24T12:02:08Z","closed_at":null,"author_association":"CONTRIBUTOR","pull_request":{"url":"https://api.github.com/repos/vaadin/flow/pulls/3029","html_url":"https://github.com/vaadin/flow/pull/3029","diff_url":"https://github.com/vaadin/flow/pull/3029.diff","patch_url":"https://github.com/vaadin/flow/pull/3029.patch"},"body":"\n\n<!-- Reviewable:start -->\nThis change is [<img src=\"https://reviewable.io/review_button.svg\" height=\"34\" align=\"absmiddle\" alt=\"Reviewable\"/>](https://reviewable.io/reviews/vaadin/flow/3029)\n<!-- Reviewable:end -->\n"},"comment":{"url":"https://api.github.com/repos/vaadin/flow/issues/comments/346813657","html_url":"https://github.com/vaadin/flow/pull/3029#issuecomment-346813657","issue_url":"https://api.github.com/repos/vaadin/flow/issues/3029","id":346813657,"user":{"login":"torok-peter","id":31210544,"avatar_url":"https://avatars3.githubusercontent.com/u/31210544?v=4","gravatar_id":"","url":"https://api.github.com/users/torok-peter","html_url":"https://github.com/torok-peter","followers_url":"https://api.github.com/users/torok-peter/followers","following_url":"https://api.github.com/users/torok-peter/following{/other_user}","gists_url":"https://api.github.com/users/torok-peter/gists{/gist_id}","starred_url":"https://api.github.com/users/torok-peter/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/torok-peter/subscriptions","organizations_url":"https://api.github.com/users/torok-peter/orgs","repos_url":"https://api.github.com/users/torok-peter/repos","events_url":"https://api.github.com/users/torok-peter/events{/privacy}","received_events_url":"https://api.github.com/users/torok-peter/received_events","type":"User","site_admin":false},"created_at":"2017-11-24T12:02:08Z","updated_at":"2017-11-24T12:02:08Z","author_association":"CONTRIBUTOR","body":"\n\n\n\nReview status: all files reviewed at latest revision, 5 unresolved discussions.\n\n---\n\n*[flow-components-parent/flow-components-test/src/test/java/com/vaadin/flow/tests/components/iron/list/IronListViewIT.java, line 45 at r1](https://reviewable.io:443/reviews/vaadin/flow/3029#-KzhEcSQfAs2Nwn7S_H5:-KzhpzGe-BS3BUvIqGE4:b-v0jij6) ([raw file](https://github.com/vaadin/flow/blob/b88c665577b11c6de986469a1bca245037c90fb2/flow-components-parent/flow-components-test/src/test/java/com/vaadin/flow/tests/components/iron/list/IronListViewIT.java#L45)):*\n<details><summary><i>Previously, gilberto-torrezan (Gilberto Torrezan) wrote…</i></summary><blockquote>\n\nThree tests use the same structure, so I preferred to create a single method for them all, just changing the id of the tested elements. Aside from adding comments to it, do you suggest any other refactoring?\n</blockquote></details>\n\nEliminating duplication is great, I like that.\n\nTo make it more readable, you could extract the 3 stages of the test into methods named e.g. `testRemovingSingleItemFromList`, `testAddingSingleItemToList` and `testRemovingAllItemsFromList`, then call these directly, effectively inlining `testListOrDataProvider`. Granted, 3 lines of code is a bit more verbose than 1, but I think that's a fair price for readable code. Having a single method named like `testRemovingSingleItemThenAddingSingleItemFromListAndFinallyRemovingAllItemsFromList` would be a bit too much for my eyes, and it wouldn't even fit into an 80 character line :-)\n\n---\n\n*[flow-components-parent/flow-components-test/src/test/java/com/vaadin/flow/tests/components/iron/list/IronListViewIT.java, line 73 at r1](https://reviewable.io:443/reviews/vaadin/flow/3029#-KzhFAJ3i1Nk1lmM5b5N:-KzhqCyA4LQLgB4uyI58:b-xbplx3) ([raw file](https://github.com/vaadin/flow/blob/b88c665577b11c6de986469a1bca245037c90fb2/flow-components-parent/flow-components-test/src/test/java/com/vaadin/flow/tests/components/iron/list/IronListViewIT.java#L73)):*\n<details><summary><i>Previously, gilberto-torrezan (Gilberto Torrezan) wrote…</i></summary><blockquote>\n\n`Assert.equals` gives a comprehensive error message already, do you want extra messages for each of the equals?\n</blockquote></details>\n\nThat's the Flow convention AFAIK. And it does make it easier to identify which assertion failed in a failing test case, which would be handy here as you have several assertions in each test.\n\n---\n\n*[flow-components-parent/flow-components-test/src/test/java/com/vaadin/flow/tests/components/iron/list/IronListViewIT.java, line 159 at r1](https://reviewable.io:443/reviews/vaadin/flow/3029#-KzhEB4x3gegbPkNZXXU:-KzhtrqUTlm74kJ0ij9V:b-ppk7ii) ([raw file](https://github.com/vaadin/flow/blob/b88c665577b11c6de986469a1bca245037c90fb2/flow-components-parent/flow-components-test/src/test/java/com/vaadin/flow/tests/components/iron/list/IronListViewIT.java#L159)):*\n<details><summary><i>Previously, torok-peter (Péter Török) wrote…</i></summary><blockquote>\n\nuse more descriptive names\n</blockquote></details>\n\nsuch as `removeAnItemButton` etc.\n\n---\n\n\n*Comments from [Reviewable](https://reviewable.io:443/reviews/vaadin/flow/3029)*\n<!-- Sent from Reviewable.io -->\n"}}
{"actor":{"display_login":"torok-peter"}}