Page MenuHomePhabricator

Tools in the toolbar do not have unique attributes
Closed, ResolvedPublic

Description

This is a huge problem for test automation.

  • open any page in VisualEditor
  • toolbar tools are located in <div class="oo-ui-toolbar-tools">
  • as far as I can see, there is no unique way to access a tool in the toolbar (example: click Insert tool)
  • all of them share the same classes, and none of them have unique attributes, like id
  • the only thing that is different is the string (Cite, Insert...), but since it changes when the UI language changes, it is not usable for us
  • our test automation breaks all the time because elements in the toolbar move around, and the workarounds that we have at the moment are all fragile
  • the best solution for us would be if every tool in the toolbar (undo, redo, paragraph...) had an unique id
  • unique classes (or any other attribute, as long as it is unique) would work for us
  • if there is an attribute used for translations, it would work for us, as long as each tool has and unique attribute
  • this is not just a problem in toolbar, but let's start somewhere

Event Timeline

zeljkofilipin raised the priority of this task from to Needs Triage.
zeljkofilipin updated the task description. (Show Details)
zeljkofilipin added a project: VisualEditor.
zeljkofilipin changed Security from none to None.
zeljkofilipin added subscribers: zeljkofilipin, Amire80.

Change 174666 had a related patch set uploaded (by Zfilipin):
WIP: Add a class to the toolbar button

https://gerrit.wikimedia.org/r/174666

Patch-For-Review

The above patch adds class ve-ui-TargetToolbar-insert to the Insert element. Something like this is all we need so far, if VisualEditor team thinks this is the way to go.

Before:

<div class="oo-ui-widget oo-ui-widget-enabled oo-ui-toolGroup oo-ui-indicatorElement oo-ui-labelElement oo-ui-popupToolGroup oo-ui-listToolGroup">

After:

<div class="ve-ui-TargetToolbar-insert oo-ui-widget oo-ui-widget-enabled oo-ui-toolGroup oo-ui-indicatorElement oo-ui-labelElement oo-ui-popupToolGroup oo-ui-listToolGroup">

Tested in MediaWiki-Vagrant virtual machine with VisualEditor role enabled.

Jdforrester-WMF added a subscriber: Jdforrester-WMF.

I'm not sure I agree with the premise here, that bloating the DOM model and so taking up more memory/etc. is a worthwhile objective just so we can test more easily… Also, if this is a requirement, it should be made in OOUI which ships the toolbar code, not hacked in by one of its clients. Adding the request to the OOUI project.

Jdforrester-WMF triaged this task as Lowest priority.Nov 29 2014, 2:38 AM
Jdforrester-WMF moved this task from To Triage to Freezer on the VisualEditor board.

I'm not sure I agree with the premise here, that bloating the DOM model and so taking up more memory/etc. is a worthwhile objective just so we can test more easily…

This is not about testing more easily, this is about having robust test automation. At the moment, our test automation is made up of cardboard and duct tape.

I'm not sure I agree with the premise here, that bloating the DOM model and so taking up more memory/etc. is a worthwhile objective just so we can test more easily…

I find it hard to believe that we still have to fight for testability in 2014. If VisualEditor team does not care about it, I give up on test automation of VisualEditor.

I'm not sure I agree with the premise here, that bloating the DOM model and so taking up more memory/etc. is a worthwhile objective just so we can test more easily…

Do you have any data on how much memory/etc. would be taken by adding a few strings to the page?

I'm not sure I agree with the premise here, that bloating the DOM model and so taking up more memory/etc. is a worthwhile objective just so we can test more easily…

This is not about testing more easily, this is about having robust test automation. At the moment, our test automation is made up of cardboard and duct tape.

If you want robust tests, agree to my request to have the browser tests block merge. :-) Right now the tests are a basket case because they don't need to pass in master, which means they often don't. The responsibility for testing software should be with the team building it, and when they break a test they should also fix it. By having browser tests not block merge, they're just out-of-band chaff.

I'm not sure I agree with the premise here, that bloating the DOM model and so taking up more memory/etc. is a worthwhile objective just so we can test more easily…

I find it hard to believe that we still have to fight for testability in 2014. If VisualEditor team does not care about it, I give up on test automation of VisualEditor.

And I find it hard to believe that I still have to argue that we should have more rigorous testing, rather than a series of hacks. :-(

Also, if this is a requirement, it should be made in OOUI which ships the toolbar code, not hacked in by one of its clients. Adding the request to the OOUI project.

As far as I am concerned, this is a requirement. Amir and I did the best we could be we were pretty sure we did it wrong. Thanks for letting us know where this belongs to.

If you want robust tests, agree to my request to have the browser tests block merge. :-) Right now the tests are a basket case because they don't need to pass in master, which means they often don't. The responsibility for testing software should be with the team building it, and when they break a test they should also fix it. By having browser tests not block merge, they're just out-of-band chaff.

Amen. It would mean some serious work on the tests, but I think this is the only way to go forward. I am not sure I have ever voted -1 when we were talking about making browser tests voting, but my memory could be failing me.

And I find it hard to believe that I still have to argue that we should have more rigorous testing, rather than a series of hacks. :-(

I am not sure what you mean here.

I think there is no risk in adding a couple of strings causing any noticeable performance problem. However, if this is how we are going to target things, than I can see this becoming a very common thing, and it won't be a couple of strings, it will be a ton of them.

My worry isn't even that it's "bloat" as much as cruft. If the classes are for testing only, then ideally we can namespace them as such. not only is "ve-ui-TargetToolbar-insert" not following the lowerCamelCase naming convention, but it's misleading that it's a class used for styling the ve.ui.TargetToolbar's insert object, rather than targeting it for testing.

I recommend we better quarantine such added classes, such that they are easy to grep for later on, should we come up with a less "duct tape" approach. Perhaps you could namespace testing classes as "ve-test" and describe the actual thing being tested with the class name, such as "ve-test-insertToolGroup"?

I expected that the names that I gave may have to be changed. I'm generally fine with any naming scheme. If everybody is sure that these things are useful for nothing but testing then it can be ve-test-*. I can imagine that these classes can also be useful for community-developed gadgets that do Stuff with the toolbar; there were similar gadgets for WikiEditor. Also, it's kinda odd to have a class with "test" in it in usual production version, but that isn't a disaster either.

The problem with letting people use these classes for gadgets is they then become an undocumented API. Trevor's point about greppability is that I should be able to see a class in the code and search for it to find out who uses it and what for, this is why we add comments when classes are built dynamically. API's for gadgets should be provided directly via JavaScript, not classnames/ID. ve-test-* obviously smells - but it is clear what it does and obvious what needs to be replaced when we come up with a better API for this particular task.

A 'better API' in this case would probably be giving groups unique symbolic names, then they could be accessed via

ve.init.target.getToolbar().getGroup( 'structure' )

for example.

I updated the patch. Now it adds classes only to elements that cannot be found in the DOM by Selenium in any other way. Incidentally, all of these are elements that are identified by a text label, rather than an icon. Where as icons have classes that are the same in all languages, the labels are different everywhere.

I think that the current version of https://gerrit.wikimedia.org/r/#/c/174666/ (Patch set 15) addresses all the comments above.

Thanks Amir. On a separate issue - would you be able to work with a JS API in this case, so we know if it's worth implementing?

Thanks Amir. On a separate issue - would you be able to work with a JS API in this case, so we know if it's worth implementing?

This is probably a question for @zeljkofilipin. For the sake of language screenshots and testing in general it must be something that will work with Ruby and Selenium, and Zeljko is a far bigger expert in this than I am.

Elitre added a subscriber: Elitre.Jan 2 2015, 6:11 PM

Change 174666 merged by jenkins-bot:
Add classes to toolbar UI elements

https://gerrit.wikimedia.org/r/174666

Amire80 closed this task as Resolved.Jan 23 2015, 4:40 AM
Jdforrester-WMF moved this task from Backlog to Reviewing on the OOUI board.Mar 26 2015, 9:01 PM