Conversation
@huangp, please review this. |
@zanata-jenkins retest this please |
.findElement( | ||
By.id("settings-permissions-form:maintainerAutocomplete:maintainerAutocomplete-result")) | ||
.findElements( | ||
By.xpath(".//ul[@class='autocomplete__results']/li")); |
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.
Looks like now we heavily rely on css class and xpath to locate elements on page. A bit worry in turns of performance and maintenance. But I don't know if there is a better way to address this @djansen-redhat . For this specific method, I will suggest extract the By.xpath(".//ul[@Class='autocomplete__results']/li") object to either an instance field or local variable or even a field in BasePage/AbstractPage if this is common for all search auto complete. Or extract the whole search auto complete as a static method in WebElementUtil.
…i?id=1066796 Squashed commit of the following: commit 655a81f Merge: 1e0829a 6724cf1 Author: Alex Eng <aeng@redhat.com> Date: Mon Mar 24 15:21:08 2014 +1000 Merge branch 'integration/master' into project-page commit 1e0829a Author: Alex Eng <aeng@redhat.com> Date: Mon Mar 24 15:20:40 2014 +1000 fix functional test commit 8b1a798 Author: Alex Eng <aeng@redhat.com> Date: Mon Mar 24 15:19:22 2014 +1000 new global message in template.xhtml commit 4186053 Author: Alex Eng <aeng@redhat.com> Date: Mon Mar 24 13:18:12 2014 +1000 fix flash scope messages, remove invalid '>' in 'No language in this version' note commit efbf11c Merge: 19e9a95 c1c04a2 Author: Alex Eng <aeng@redhat.com> Date: Fri Mar 21 11:23:20 2014 +1000 Merge branch 'integration/master' into project-page Conflicts: zanata-war/src/main/webapp/WEB-INF/layout/version-group/languages-tab.xhtml zanata-war/src/main/webapp/WEB-INF/layout/version-group/projects-tab.xhtml zanata-war/src/main/webapp/WEB-INF/layout/version-group/settings-tab.xhtml zanata-war/src/main/webapp/resources/script/components-script.js zanata-war/src/main/webapp/version-group/version_group.xhtml commit 19e9a95 Author: Alex Eng <aeng@redhat.com> Date: Fri Mar 21 08:59:12 2014 +1000 fix checkstyle commit c3d40f0 Author: Alex Eng <aeng@redhat.com> Date: Fri Mar 21 07:35:27 2014 +1000 Fix document list not update after delete commit 7191963 Author: Alex Eng <aeng@redhat.com> Date: Thu Mar 20 16:56:55 2014 +1000 WIP: document update commit 716a96c Author: Alex Eng <aeng@redhat.com> Date: Thu Mar 20 11:47:04 2014 +1000 Add loading icon on right panel, changed of badge to label for text commit 7ce92e1 Author: Alex Eng <aeng@redhat.com> Date: Wed Mar 19 00:09:06 2014 +1000 Handle disabled locale in list commit 7d818bc Author: Alex Eng <aeng@redhat.com> Date: Tue Mar 18 16:18:09 2014 +1000 Redirect source files and files url to version's document tab commit c58ecb1 Author: Alex Eng <aeng@redhat.com> Date: Tue Mar 18 13:36:05 2014 +1000 Move repeated comparator to ComparatorUtil collection commit 1b11eb9 Author: Alex Eng <aeng@redhat.com> Date: Tue Mar 18 12:23:38 2014 +1000 Refactor code according to review comments commit 14412c0 Author: Alex Eng <aeng@redhat.com> Date: Tue Mar 18 08:01:38 2014 +1000 Refactor list-filter component commit 2524ba4 Author: Alex Eng <aeng@redhat.com> Date: Mon Mar 17 16:54:41 2014 +1000 New project and version page: https://bugzilla.redhat.com/show_bug.cgi?id=1066796
@aeng Not sure if you are aware this is failing tests. |
* Shift focus to the page, in order to activate some elements that only | ||
* exhibit behaviour on losing focus. Some pages with contained objects | ||
* cause object behaviour to occur when interacted with, so in this case | ||
* interact with the container instead. |
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.
Lets try to avoid these changes, they waste time specially on long pull requests.
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.
auto format causing this.
Projects and ProjectVersions have changed significantly and the previous test suite was somewhat conceptually outdated. Move settings pages to tabs. Add some project tests. Seperate projects and versions.
@In | ||
private ZanataIdentity identity; | ||
|
||
@Getter | ||
private final AbstractAutocomplete<SearchResult> projectAutocomplete = | ||
new AbstractAutocomplete<SearchResult>() { |
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.
Might be more readable to not have this as an anonymous class, even if it's just a nested one.
if (flashScopeMessage == null) { | ||
flashScopeMessage = FlashScopeMessage.instance(); | ||
} | ||
return flashScopeMessage; |
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.
I just noticed this method which happens to be in several other classes. Why fetch the component when you are injecting it? Seems to break the whole inversion of control paradigm.
Collection<HPerson> filtered = | ||
Collections2.filter(unfiltered, new Predicate<HPerson>() { | ||
@Override | ||
public boolean apply(@Nullable HPerson input) { |
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.
And here
} | ||
|
||
@SuppressWarnings("unchecked") | ||
private List<HTextFlowTargetHistory> getHistory(HTextFlowTarget tft) { | ||
return getSession().createCriteria(HTextFlowTargetHistory.class) | ||
.add(Restrictions.eq("textFlowTarget", tft)).list(); | ||
.add(Restrictions.eq("textFlowTarget", tft)).list(); |
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.
This file needs to reformat
Significant changes to the structure and behaviour of project and version tests.
@Jenkins retest this please |
1 similar comment
@Jenkins retest this please |
If you merge in the latest master, there's a fix for the test GetLocaleListHandlerTest.testExecuteWithOverriddenIterationLocales. |
Conflicts: zanata-war/src/main/java/org/zanata/action/ViewAllStatusAction.java zanata-war/src/main/webapp/resources/script/components-script.js zanata-war/src/test/java/org/zanata/webtrans/server/rpc/GetLocaleListHandlerTest.java
Just curious, why we are doing refactoring in midst of development? Won't this only cause unnecessary conflicts? |
@Jenkins retest this please |
@Jenkins retest this please |
1 similar comment
@Jenkins retest this please |
On 2014-04-04 16:06, Alex Eng wrote:
We're always in the midst of development! I didn't know you would be The old test was broken because it was testing the order of an unordered Even if I had fixed the order testing using Hamcrest, there would have Sean Flanigan Senior Software Engineer |
This message has changed, but will change again in a different pull request. Removed the test assert for it.
I think I've beaten this request enough. |
New project and version page
https://bugzilla.redhat.com/show_bug.cgi?id=1066796