Add gtest for AccessibleCaretManager

RESOLVED FIXED in Firefox 43

Status

()

defect
RESOLVED FIXED
4 years ago
2 months ago

People

(Reporter: TYLin, Assigned: TYLin)

Tracking

(Blocks 1 bug)

unspecified
mozilla43
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox43 fixed)

Details

Attachments

(5 attachments)

Assignee

Description

4 years ago
Currently, there's no unit test for the appearance of the two AccessibleCarets after being manipulated by AccessibleCaretManager. Moreover, the root cause of some Gaia issues like bug 1196176 is that gecko does not send the necessary CaretStateChanged events. We should have gtest to cover that.
Assignee

Comment 1

4 years ago
Bug 1204872 - Add documentation and rename mCaretMode. r=roc

Rename mCaretMode to mLastUpdateCaretsMode to make it clear that this
variable track the caret mode since last update. This also avoid the
confusion that GetCaretMode() returns mCaretMode.
Attachment #8661268 - Flags: review?(roc)
Assignee

Comment 2

4 years ago
Bug 1204872 - Make AccessibleCaret testable. r=roc

Remove the assert of the existence of PresShell in the constructor since
there's no PresShell in gtest. Also make AccessibleCaret inheritable.
Attachment #8661269 - Flags: review?(roc)
Assignee

Comment 3

4 years ago
Bug 1204872 - Make AccessibleCaretManager testable. r=roc

We need to extract statements that touch PresShell or access utilities
into functions so that we could mock them in gtest.
Attachment #8661270 - Flags: review?(roc)
Assignee

Comment 4

4 years ago
Bug 1204872 - Prettify enum class printing. r=roc

Provide operator<< functions so that gtest can use them to print enum
classes.
Attachment #8661271 - Flags: review?(roc)
Assignee

Comment 5

4 years ago
Bug 1204872 - Add gtest for AccessibleCaretManager. r=roc

Add basic gtest to ensure AccessibleCaret and AccessibleCaretManager are
both testable. More tests to come.
Attachment #8661272 - Flags: review?(roc)
Comment on attachment 8661268 [details]
MozReview Request: Bug 1204872 - Add documentation and rename mCaretMode. r=roc

https://reviewboard.mozilla.org/r/19321/#review17335
Attachment #8661268 - Flags: review?(roc) → review+
Comment on attachment 8661269 [details]
MozReview Request: Bug 1204872 - Make AccessibleCaret testable. r=roc

https://reviewboard.mozilla.org/r/19323/#review17341
Attachment #8661269 - Flags: review?(roc) → review+
Comment on attachment 8661270 [details]
MozReview Request: Bug 1204872 - Make AccessibleCaretManager testable. r=roc

https://reviewboard.mozilla.org/r/19325/#review17343

It makes me sad we have to make functions virtual in release builds just for testing.

Please add comments for each such function saying that it's virtual for testing purposes.
Attachment #8661270 - Flags: review?(roc) → review+
Assignee

Comment 11

4 years ago
https://reviewboard.mozilla.org/r/19325/#review17343

To avoid run time overhead by virtual functions in production, how about we move those function into a helper class, say AccessibleCaretUtility, and make a template parameter for AccessibleCaretManager to use it? In production we can construct AccessibleCaretManager<AccessibleCaretUtility>. And in testing, we can provide a MockAccessibleCaretUtility with the same signatures as AccessibleCaretUtility, and construct it by using AccessibleCaretManager<MockAccessibleCaretUtility>.

https://code.google.com/p/googlemock/wiki/CookBook#Mocking_Nonvirtual_Methods
Assignee

Updated

4 years ago
Flags: needinfo?(roc)
That sounds good!
Flags: needinfo?(roc)
Assignee

Comment 13

4 years ago
Comment on attachment 8661268 [details]
MozReview Request: Bug 1204872 - Add documentation and rename mCaretMode. r=roc

Bug 1204872 - Add documentation and rename mCaretMode. r=roc

Rename mCaretMode to mLastUpdateCaretsMode to make it clear that this
variable track the caret mode since last update. This also avoid the
confusion that GetCaretMode() returns mCaretMode.
Assignee

Comment 14

4 years ago
Comment on attachment 8661269 [details]
MozReview Request: Bug 1204872 - Make AccessibleCaret testable. r=roc

Bug 1204872 - Make AccessibleCaret testable. r=roc

Remove the assert of the existence of PresShell in the constructor since
there's no PresShell in gtest. Also make AccessibleCaret inheritable.
Assignee

Comment 15

4 years ago
Comment on attachment 8661270 [details]
MozReview Request: Bug 1204872 - Make AccessibleCaretManager testable. r=roc

Bug 1204872 - Make AccessibleCaretManager testable. r=roc

We need to extract statements that touch PresShell, access frame trees,
or call other utility functions into functions so that we could mock or
stub them in gtest.

Inline GetFocusedContent since it's only used once.
Assignee

Comment 16

4 years ago
Comment on attachment 8661271 [details]
MozReview Request: Bug 1204872 - Prettify enum class printing. r=roc

Bug 1204872 - Prettify enum class printing. r=roc

Provide operator<< functions so that gtest can use them to print enum
classes.
Assignee

Comment 17

4 years ago
Comment on attachment 8661272 [details]
MozReview Request: Bug 1204872 - Add gtest for AccessibleCaretManager. r=roc

Bug 1204872 - Add gtest for AccessibleCaretManager. r=roc

Add basic gtest to ensure AccessibleCaret and AccessibleCaretManager are
both testable. More tests to come.
Assignee

Comment 18

4 years ago
Re comment #8 and comment #11:

> Please add comments for each such function saying that it's virtual for
> testing purposes.

On second thought, I think it might not be worth adding a class template parameter for gtest since it will touch too many code. The code for AccessibleCaret isn't that hot. Virtual function calls should be bearable. I've upload a new patch set to group those virtual functions together on the header, and add comments saying that they're virtual for testing.
Assignee

Comment 19

4 years ago
Comment on attachment 8661268 [details]
MozReview Request: Bug 1204872 - Add documentation and rename mCaretMode. r=roc

Bug 1204872 - Add documentation and rename mCaretMode. r=roc

Rename mCaretMode to mLastUpdateCaretsMode to make it clear that this
variable track the caret mode since last update. This also avoid the
confusion that GetCaretMode() returns mCaretMode.
Assignee

Comment 20

4 years ago
Comment on attachment 8661269 [details]
MozReview Request: Bug 1204872 - Make AccessibleCaret testable. r=roc

Bug 1204872 - Make AccessibleCaret testable. r=roc

Remove the assert of the existence of PresShell in the constructor since
there's no PresShell in gtest. Also make AccessibleCaret inheritable.
Assignee

Comment 21

4 years ago
Comment on attachment 8661270 [details]
MozReview Request: Bug 1204872 - Make AccessibleCaretManager testable. r=roc

Bug 1204872 - Make AccessibleCaretManager testable. r=roc

We need to extract statements that touch PresShell, access frame trees,
or call other utility functions into functions so that we could mock or
stub them in gtest.

Inline GetFocusedContent since it's only used once.
Assignee

Comment 22

4 years ago
Comment on attachment 8661271 [details]
MozReview Request: Bug 1204872 - Prettify enum class printing. r=roc

Bug 1204872 - Prettify enum class printing. r=roc

Provide operator<< functions so that gtest can use them to print enum
classes.
Assignee

Comment 23

4 years ago
Comment on attachment 8661272 [details]
MozReview Request: Bug 1204872 - Add gtest for AccessibleCaretManager. r=roc

Bug 1204872 - Add gtest for AccessibleCaretManager. r=roc

Add basic gtest to ensure AccessibleCaret and AccessibleCaretManager are
both testable. More tests to come.

Allow -Winconsistent-missing-override warning in gtest since MOCK_METHOD
does not have 'override' keyword. See bug 1169974.
You need to log in before you can comment on or make changes to this bug.