Problem/Motivation
(Rewritten after request on comment #10.)
We need to test top-level tab creation separately from tray creation, when using hook_toolbar() to do so.
The current tests use toolbar_test.module to only test returning both a top-level tab and a tray together, so not covering the full range of behaviours.
Proposed resolution
Because any given module's hook_toolbar()
always returns the same array of data, we need multiple implementations to test multiple scenarios.
- Rename module
toolbar_test
totoolbar_test_tab_and_tray
. - (Maybe run this through
coder-review
when doing so, as e.g. indentation seems a little off?) - Clone it to create two new modules
toolbar_test_tab
andtoolbar_test_tray
, and edit down theirhook_toolbar()
implementations. - Similarly with
ToolbarHookToolbarTest
, rename and clone, to test now threedifferent toolbar_test_*
modules.
Remaining tasks
Task | Novice task? | Contributor instructions | Complete? |
---|---|---|---|
Update the issue summary | Instructions | Yes | |
Create a patch | Instructions | ||
Add automated tests | Instructions |
User interface changes
None: tabs and trays are only added during running of tests.
API changes
None: new tests are only autodiscovered during running of tests.
Comment | File | Size | Author |
---|---|---|---|
#29 | 1839514-27.patch | 1.66 KB | Nikhil_110 |
| |||
#16 | expand_test_coverage-1839514-16.patch | 1.57 KB | dimaro |
#13 | 1839514-13.patch | 1.52 KB | olafkarsten |
Comments
Comment #1
benjifisher@jessebeach:
In #1137920: Fix toolbar on small screen sizes and redesign toolbar for desktop you have been saying that "benjifisher is working on" this issue. Not yet. I will get to it soon. (Too many comments and too many followers on that issue, and I do not want to add to the overload.)
If anyone else wants to work on it, I promise to post a comment when I start. I hate duplicated effort.
I can exercise
hook_toolbar()
andhook_toolbar_alter()
. I would appreciate help, advice, or pointers on how to test the javascript.Comment #2
jessebeach CreditAttribution: jessebeach commentedNo need to address JavaScript testing, just simpletests of PHP. We'll be adding JS tests after code freeze.
Comment #3
Shyamala CreditAttribution: Shyamala commentedediting tags
Comment #4
YesCT CreditAttribution: YesCT commentedI was looking for a simpletest from when the toolbar went in, that tested for icons, so I could copy it for this language issue:
#1848552: Toolbar icons disappear with translated menu
It sounds like there are not tests yet?
Comment #5
benjifisherI am working on rewriting
hook_toolbar()
in #1847198: Update the structure returned by hook_toolbar(). I will submit patches here when that work is done. For now, I have set up a sandbox project for a testing/example module for the toolbar hooks: http://drupal.org/sandbox/benji/1862108.I would like to have help on the sandbox project from someone who knows CSS, icons, and javascript better than I do.
Comment #6
benjifisherThe existing tests are being updated as part of #1847198: Update the structure returned by hook_toolbar(). I think it makes sense to wait until that issue is resolved before adding any more, so I am marking this issue Postponed.
According to the discussion in #1532612: Move Examples Project into Drupal core, we should do much more than the tests suggested in the issue summary. We should test every edge case we can imagine and do our best to break the API.
Comment #6.0
benjifisherUpdated issue summary.
Comment #7
mgiffordComment #8
Wim LeersWe have this now, was added in #1847198: Update the structure returned by hook_toolbar().
We don't have
yet.
Updating accordingly.
Comment #9
Wim LeersComment #10
YesCT CreditAttribution: YesCT commentedI think this needs an issue summary update in order for a new contributor to understand what needs to be done.
https://drupal.org/core-mentoring/novice-tasks helps people decide what is a novice task and what is not
Comment #11
jp.stacey CreditAttribution: jp.stacey at Magnetic Phield commentedLooking at this as part of Sprint Weekend 2016.
Comment #12
jp.stacey CreditAttribution: jp.stacey at Magnetic Phield commentedUpdated issue summary. I'm happy to work on this if the proposed solution looks right, but given I've expanded a lot on the original description it would be good to get some other eyes on it before I go off on a tangent!
Comment #13
olafkarsten CreditAttribution: olafkarsten as a volunteer commentedI don't think we need a bunch of separate modules here. We could simple expand the existing test. Or do I miss something?
Here is my try.
Comment #16
dimaro CreditAttribution: dimaro at La Drupalera by Emergya commentedPatch no longer applies against 8.4.x
Reroll #13 against 8.4.x
Does the proposed solution remain the same as the indicated in the issue description?
Comment #28
smustgrave CreditAttribution: smustgrave at Mobomo commentedThis issue is being reviewed by the kind folks in Slack, #need-reveiw-queue. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge require as a guide.
The tasks in the IS
From what I can tell these haven't happened.
Comment #29
Nikhil_110 CreditAttribution: Nikhil_110 at Srijan | A Material+ Company commentedAttached patch against drupal 10.1.x