Remove reflective access from find/replace tests #2060

[HeikoKlare]

The tests for the FindReplaceDialog and FindReplaceOverlay currently use reflection to access specific UI elements. This ties the test implementations to implementation details of the production classes (i.e., specific hidden field to be present) and particularly requires the production code to contain (hidden) fields even if they would not be required just to provide according tests.

This change replaces the reflective access (at the code level) with widget extraction functionality based on unique information about the widget to be searched for. This may also be considered reflective (at the configuration level), but at least it gets rid of requiring the code to contain specific fields just in order to write tests that need to extract them.

Fixes #2060

This is also a preparation for:

• Find/replace overlay: replace shell with integrated composite #2099 #2254

[laeubi]

@HeikoKlare A usual way is to specify an ID in the Widget#setData (see also eclipse-platform/eclipse.platform.swt#549 ) so one can identify them in test. SWT Bot on the other hand also contains some useful helper methods in this regards.

[github-actions]

Test Results

 1 815 files  ±0   1 815 suites  ±0   1h 31m 13s ⏱️ - 3m 28s
 7 702 tests ±0   7 462 ✅ ±0  228 💤 ±0  1 ❌ ±0  11 🔥 ±0 
24 267 runs  ±0  23 508 ✅ ±0  747 💤 ±0  1 ❌ ±0  11 🔥 ±0 

For more details on these failures and errors, see this check.

Results for commit 8e94bff. ± Comparison against base commit 9b21618.

♻️ This comment has been updated with latest results.

[Wittmaxi]

I do not yet see the added benefit here - can you please elaborate a bit?

I understand that we want to get rid of reflection. The reason we don't like reflection is because it adds a new predicate for the code to run - i.e. changing the class structure of FindReplaceOverlay changes the behavior of FindReplaceOverlayTest as a side-effect.
This change makes the test rely on the text that is set to the FindReplaceOverlay, instead of the class structure - i.e. changing the text of the FindReplaceOverlay changes the behavior of FindReplaceOverlayTest as a side-effect.

This effectively means that we get rid of one dependency to introduce another dependency.
Since the class is coupled with the test, it a future developer changing the FindReplaceOverlay class would see a test failing and think "oh, that makes sense. The design is bad, but I understand why this test fails".
On the other hand, having a unit test fail because I change the text of a tooltip might not be very intuitive for me, if I am not familiar with the code. This gets even worse if we hire e.g. a copywriter to write texts (... far fetched, sure).

@laeubi's approach of setting IDs sounds more sound to me, but I think I am missing some information here: what is the (true) problem that we are trying to solve here?

[HeikoKlare]

I do not yet see the added benefit here - can you please elaborate a bit?

The benefit is that production code does not need to be aligned with test code anymore. Currently, production code needs to have the fields that are reflectively accessed to make the test work. With the proposed change, that's different as the tests use production information (texts, tooltips) to identify some widget. So yes, if you change the production code you may need to adapt tests (but that's somehow expected). But you do not need to keep otherwise unnecessary production code (e.g., unused fields) just to make the tests work.

usual way is to specify an ID in the Widget#setData (see also eclipse-platform/eclipse.platform.swt#549 ) so one can identify them in test. SWT Bot on the other hand also contains some useful helper methods in this regards.

In general, that seems to be a better approach as you tie to IDs instead of NLS, which are more prone to change. I am just a bit concerned to introduce IDs only for these UI elements, because that would make it similar to the existing approach as the IDs would not serve any other purpose than implementing tests: production code is adapted to test code (which requires IDs) instead of the other way round. I think we would need to have a general pattern for such IDs to rely on by convention instead of introducing a specific convention for this use case. So all approaches have drawbacks, but maybe adding IDs is the least bad one as it should at least be stable.

With respect to SWTBot, I remember a discussion that we do not want to use it in the platform projects (I think it's not even available via the TP), which is why I avoided using it. In general, having a proper UI testing framework instead of multiple use-case-specific solutions would of course be preferable.

[Wittmaxi]

@HeikoKlare

So all approaches have drawbacks, but maybe adding IDs is the least bad one as it should at least be stable.

The stability of the id-approach is what sells it for me.

I had tried SWTBot and RedDeer and I am confident that neither are helpful to us. I remember having documented the drawbacks of either in an internal chat (I believe it was related to 1) deprecation 2) problems with inclusion into eclipse 3) neither really "worked")

[laeubi]

In general, that seems to be a better approach as you tie to IDs instead of NLS, which are more prone to change. I am just a bit concerned to introduce IDs only for these UI elements

Currently your test uses IDs already (you can also call it a "name" if you feel more comfortable with that), the NLS could probably change as well, it was just a hint that assign data (it must not be an String!) is often a very powerful approach that decouples the provider from the consumer, e.g. JFace uses that to connect the wrapper classes to the widget.

[HeikoKlare]

You are right, the current proposal also uses IDs. Only difference is that the used ID information is required by production code and thus present anyway (as the widgets need to have some name/tooltip/...) while an "explicit" ID will (currently) only be added for tests. But it can, of course, also be used for other purposes. I will adapt the PR accordingly.

[laeubi]

Suggested change

	FindReplaceOverlay overlay= (FindReplaceOverlay) actionAccessor.get("overlay");
	FindReplaceOverlay overlay= (FindReplaceOverlay) viewer.getControl().getData(FindReplaceOverlay.class.getName()

Just as an example on how one can "transport" an object through the data of the underlying control, of course the FindReplaceOverlay needs to set the data accordingly once it is installed in a TextViewer and remove itself once it is removed. That way it is very easy to check if a given TextViewer has a FindReplaceOverlay or not (not only for tests).

If this is needed on multiple places one can even has

static Optional<FindReplaceOverlay> FindReplaceOverlay.of(TextViewer viewer)

that encapsulates the details on how a FindReplaceOverlay know its connection to a TextViewer.

[HeikoKlare]

That looks like a great idea for further reducing reflective access of the tests (in this case related to the FindReplaceAction instead of the dialog/overlay itself) and maybe can also improve the FindReplaceAction itself because it could relieve the action from tracking the overlay by just attaching it to the target text editor. I would like to follow-up on this in a subsequent PR.

[HeikoKlare]

I have now replaced the widget extraction based on NLS information with explicit IDs. Those IDs are simple strings that need to match between production code and tests as well. An alternative would be to not store the IDs in the individual widgets and traverse the widgets to identify them but to register the individual widgets with those IDs as data at the "root widget" of the overlay/dialog and directly retrieve them from that widget via those IDs. That would make the additional widget traversal logic obsolete but would then require the dialog/overlay classes to expose their root widget. With #2254 the FindReplaceOverlay will not be a dialog and not expose the root widget anymore but hide it is an implementation detail, which would then need to be exposed only for the sake of testability.

@laeubi what do you think of the current solution?

[Wittmaxi]

Great improvement of the tests! I am not 100% sure that assignIDs in FindReplaceOverlay is correct.
From my static analysis this is the current status:
For the current tests it is certainly good enough, but a future tester might(should!) assume that it assigns IDs to every component - which it currently does not necessarily. The issue arises when the replaceBar is not open while opening the overlay. Since assignIDs is only called when opening the Overlay first, it will never react to toggling the replace bar open in a later stage of the overlay usage.

[HeikoKlare]

Failing tests are unrelated and already documented: #2346