Fix API errors in SWT due to tests #1299

fedejeanne · 2024-06-24T06:56:54Z

Move Win32AutoscaleTestBase to package org.eclipse.swt
Increase minor version in manifest
Add missing @since tags in extending test classes

binaries/org.eclipse.swt.win32.win32.x86_64/META-INF/MANIFEST.MF

@SInCE

- Move Win32AutoscaleTestBase to package org.eclipse.swt - Increase minor version in manifest - Add missing @SInCE tags in extending test classes Fixes eclipse-platform#1298

github-actions · 2024-06-24T07:09:45Z

Test Results

470 files ±0 470 suites ±0 7m 50s ⏱️ -2s
4 135 tests ±0 4 127 ✅ ±0 8 💤 ±0 0 ❌ ±0
16 336 runs ±0 16 244 ✅ ±0 92 💤 ±0 0 ❌ ±0

Results for commit cb7188b. ± Comparison against base commit 23f5c83.

♻️ This comment has been updated with latest results.

iloveeclipse

Why are tests added to the production bundle in the first place?
They belong to org.eclipse.swt.tests.win32

We never host tests in production bundles in platform.

fedejeanne · 2024-06-24T07:32:10Z

Why are tests added to the production bundle in the first place? They belong to org.eclipse.swt.tests.win32

We never host tests in production bundles in platform.

This has been discussed before and the approach has been OK`d. See:

iloveeclipse · 2024-06-24T07:44:51Z

Why are tests added to the production bundle in the first place? They belong to org.eclipse.swt.tests.win32
We never host tests in production bundles in platform.

This has been discussed before and the approach has been OK`d. See:

Reduce Win32 FontRegistries visibility and move tests inside fragment #1203 (comment)

This doesn't mean the test code has to be in same package as production. Everyone can now code against this "API" (which is not).

Using test fragments for unit tests .github#203

This discussion was about fragments but the code is inside main SWT bundle

I'm not sure why the new tests are so different to every existing test that they have to be in production code.

@HeikoKlare : can we please move the tests to test bundles?

merks · 2024-06-24T07:58:37Z

I don’t think the tests end up in the production bundle do they? Let’s be fully informed before we make conclusions about actions to be taken. In my picture they are in the fragment and I do recall discussing that.

iloveeclipse · 2024-06-24T08:00:35Z

Beside this, even if the test code stays where it is today (which I hope will not happen), we should never add ANY "since" versions to it but make sure that API tooling ignores this code completely (which would happen automatically once the test code would be moved to tests).

iloveeclipse · 2024-06-24T08:04:13Z

In my picture they are in the fragment and I do recall discussing that.

I'm talking about SWT source tree right now, API errors and a major inconsistency introduced.

I don't see final classes in the main SWT bundle, but that means, everyone with SWT projects imported in the workspace doesn't see same behavior of the code & tests as we see it at the runtime tests, same for API problems. This will be a major issue and time waste in the future if we start doing that, we have enough time spent on debugging SWT API problems in the past and I don't want anyone time be wasted here again.

deepika-u · 2024-06-24T08:59:53Z

I am not sure if i can update here but still please let me know if this makes it more meaningful if updated in a different task. Thanks.

Inspite of applying this patch on the latest SWT level i am still facing the below problems. Please let me know i missed out any steps for the same to be avoided or this changeset needs more addition with respect to the below errors as well.

fedejeanne · 2024-06-24T09:16:23Z

In my picture they are in the fragment and I do recall discussing that.

I'm talking about SWT source tree right now, API errors and a major inconsistency introduced.

I don't see final classes in the main SWT bundle, but that means, everyone with SWT projects imported in the workspace doesn't see same behavior of the code & tests as we see it at the runtime tests, same for API problems. This will be a major issue and time waste in the future if we start doing that, we have enough time spent on debugging SWT API problems in the past and I don't want anyone time be wasted here again.

@iloveeclipse Can you please elaborate on that? I don't understand what you mean by "same behavior".

iloveeclipse · 2024-06-24T09:25:20Z

Can you please elaborate on that? I don't understand what you mean by "same behavior".

If a test fails or code works in the workspace it doesn't guarantee it will fail on Jenkins and vice versa.

E.g. add reference to any of the test classes to any SWT production class and it may be green locally in the workspace but I hope it will fail on Jenkins. if it will not fail on Jenkins, we have even a bigger issue.

laeubi · 2024-06-24T10:06:01Z

I don't see final classes in the main SWT bundle, but that means, everyone with SWT projects imported in the workspace doesn't see same behavior of the code & tests as we see it at the runtime tests,

In my picture they are in the fragment and I do recall discussing that.

I'm talking about SWT source tree right now, API errors and a major inconsistency introduced.

I don't see final classes in the main SWT bundle, but that means, everyone with SWT projects imported in the workspace doesn't see same behavior of the code & tests as we see it at the runtime tests, same for API problems. This will be a major issue and time waste in the future if we start doing that, we have enough time spent on debugging SWT API problems in the past and I don't want anyone time be wasted here again.

If the folder is correctly marked as a test source folder then no one will ever be able to reference anything here in regular/production code (otherwise its a bug in JDT)... that APi tools complains in the IDE might be a bug in PDE API tools that should not consider code located in folders not added in build.properties.

laeubi · 2024-06-24T10:18:24Z

Beside this, even if the test code stays where it is today (which I hope will not happen),

Just curious, but why "hope will not happen" this is common practice in the software industry, its just Eclipse (PDE) that is special here and makes it hard to use/understand for newcomers that are used to the usual layout.

iloveeclipse · 2024-06-24T11:15:16Z

Beside this, even if the test code stays where it is today (which I hope will not happen),

Just curious, but why "hope will not happen" this is common practice in the software industry

This is not correct, as Eclipse is also part of the "software industry" and here it is not common practice. To speaking "maven" speak, if maven implies project layout for good reason, Eclipse implies separation of production and test code into different projects, also for good reasons, and that for every other project in the platform since ever.

its just Eclipse (PDE) that is special here and makes it hard to use/understand for newcomers that are used to the usual layout.

It is not correct either, it is not about special PDE but about special change in question that broke consistent and easy to understand usual layout followed by any other Eclipse platform project.

My problem is: all Eclipse Platform projects followed (till now) same common standards regarding tests. It was consistent, easy to understand and supported by tooling. Now we have an exception from this rule that is, in my opinion, not justified from technical point of view, not acceptable from platform project common practice point of view and shows first problem with the common tooling we use.

This PR is just a "band-aid" for this main inconsistency, it makes no sense as the way it fixes the problem is wrong (there should be no need for the "API since" flags on non API code) and I'm against adding that and in favour of fixing original problem to avoid further problems arising from this band aid and the inconsistency.

fedejeanne · 2024-06-24T11:34:05Z

My problem is: all Eclipse Platform projects followed (till now) same common standards regarding tests. It was consistent, easy to understand and supported by tooling. Now we have an exception from this rule that is, in my opinion, not justified from technical point of view, not acceptable from platform project common practice point of view and shows first problem with the common tooling we use.

The justification from the technical point of view was provided in eclipse-platform/.github#203: it wasn't possible to test the microarchitecture of the Windows port with the current approach without elevating the classes that we wanted to test to a public API.

This PR is just a "band-aid" for this main inconsistency, it makes no sense as the way it fixes the problem is wrong (there should be no need for the "API since" flags on non API code) and I'm against adding that and in favour of fixing original problem to avoid further problems arising from this band aid and the inconsistency.

As @laeubi said in #1299 (comment)

... that APi tools complains in the IDE might be a bug in PDE API tools that should not consider code located in folders not added in build.properties.

Is a different subject.

I could close this PR if this is the wrong fix for the errors in the API verification, but the tests should remain where they are.

BTW I tried what you said about referencing the test class in production code and it is not possible (compilation fails, even for Linux)

So, summarizing:

Close this PR
Solve the problem by not checking the API of code that is not in the build.properties (@laeubi any idea how to do that?)
Leave the tests where they are (the folder is already correctly marked as test folder in .classpath_win32)

WDYT?

laeubi · 2024-06-24T12:46:11Z

This is not correct, as Eclipse is also part of the "software industry" and here it is not common practice.

Maybe not for Eclipse Platform, but anyways I don't have stated any thing that is is the only practice. Beside that it does not help to complain anyways, if no one would want/use it such feature would simply not exits.

if maven implies project layout for good reason

Maven does not imply anything but is driven by convention over configuration, that means it was agreed that it is useful for a larger scale of users, still it does not forbids anything different used, it is just more work to configure.

Eclipse implies separation of production and test code into different projects, also for good reasons, and that for every other project in the platform since ever.

Eclipse does not imply anything, also here you can do use different layouts, it is just that PDE/Tycho where limited in the past to a specific workaround that was claimed "best practice" because there was no other practices possible. But just because we are not used to it does not mean its useless/dangerous/bad/... and it is/was one of the often asked questions in the past why it is not supported by PDE/Tycho to have such layout.

And as you have already found out, the code is seperated and JDT even marks test folders with a different visual layout, and it is even more easier to match test+code while with a seperate project there is some chance to miss it (project not imported or not noticed as its is "far away" from the implementation, ...)

It is not correct either

I think we can accept different viewpoints and people are working differently so I really don't like to enter another fruitless endless "war of opinions here" there is simply no right/wrong, the only thing is that it was for a long time not possible (or very hard to archive).

My problem is: all Eclipse Platform projects followed (till now) same common standards regarding tests.

As mentioned, before it was simply not possible, as before it was not possible to use maven-deps and everyone "followed" that standard... things are changing, so maybe even if it is uncommon until now this might or might not change, but trying it out and maybe see some pitfalls that needs fixing is nothing I would worry about.

WDYT?

I think we should verify that everything is setup correctly that is:

Folder is marked as test and shows up as such
Folder is not included in build.properties

If that is the case I would expect the tooling to work and test code is neither shipped nor useable from "production" (even though some argue that test-artifacts are valuable deliveries for a product). If any issues are seen we should identify and fix them so whoever wants to use that (let it be platform or not) do not run into these as well.

If Platform should or not should use it is a completely different topic (for me) as whatever we do someone will always be unhappy... so lets focus first on the technical aspects.

merks · 2024-06-25T10:54:50Z

Discussions seem to have stalled.

It seems to me also that anything in a source folder marked as a test should also not be considered API by API tooling:

But again, this requires some fix in the API tools. Perhaps I can find time to have a look, but that's only sensible if this conversation draws to a conclusion...

laeubi · 2024-06-25T10:58:19Z

@merks I know one needs to decide where to spend time, but

anything in a source folder marked as a test should also not be considered API by API tooling

is completely independent (and at least deserves an issue at PDE), from:

this conversation draws to a conclusion...

I just would think it makes it easier to fix (and maybe helps with the discussion if solved) if we have a case to analyze now.

merks · 2024-06-25T14:11:29Z

That's a good point. Even if not for SWT, it generally makes sense that project unit tests are not API.

I experimented a bit adding this tester method:

And using it here:

and here:

eliminates the SWT errors we see:

merks · 2024-06-25T14:54:56Z

FYI, I can see this in the .api_description of the most recent 4.33 I-build:

<?xml version="1.0" encoding="UTF-8" standalone="no"?>
<component name="org.eclipse.swt.win32.win32.x86_64_3.127.0.v20240624-1000" version="1.2">
    <plugin id="org.eclipse.swt.win32.win32.x86_64_3.127.0.v20240624-1000"/>
    <package name="org.eclipse.swt.accessibility" visibility="1">
        <type name="Accessible" restrictions="0">
            <method name="internal_WM_GETOBJECT" restrictions="8" signature="(JJ)J"/>
            <method name="internal_dispose_Accessible" restrictions="8" signature="()V"/>
            <method name="internal_new_Accessible" restrictions="8" signature="(Lorg/eclipse/swt/widgets/Control;)Lorg/eclipse/swt/accessibility/Accessible;"/>
        </type>
    </package>
    <package name="org.eclipse.swt.browser" visibility="1">
        <type name="Browser" restrictions="2"/>
    </package>
    <package name="org.eclipse.swt.custom" visibility="1">
        <type name="CBanner" restrictions="2"/>
        <type name="CTabFolder" restrictions="2"/>
        <type name="CTabItem" restrictions="2"/>
        <type name="StyledText" restrictions="2"/>
        <type name="ViewForm" restrictions="2"/>
    </package>
    <package name="org.eclipse.swt.dnd" visibility="1">
        <type name="Clipboard" restrictions="2"/>
        <type name="DragSource" restrictions="2"/>
        <type name="DropTarget" restrictions="2"/>
        <type name="TransferData" restrictions="0">
            <field name="formatetc" restrictions="8"/>
            <field name="pIDataObject" restrictions="8"/>
            <field name="result" restrictions="8"/>
            <field name="stgmedium" restrictions="8"/>
            <field name="type" restrictions="8"/>
        </type>
    </package>
    <package name="org.eclipse.swt.graphics" visibility="1">
        <type name="Color" restrictions="0">
            <field name="handle" restrictions="8"/>
            <method name="win32_new" restrictions="8" signature="(Lorg/eclipse/swt/graphics/Device;I)Lorg/eclipse/swt/graphics/Color;"/>
            <method name="win32_new" restrictions="8" signature="(Lorg/eclipse/swt/graphics/Device;II)Lorg/eclipse/swt/graphics/Color;"/>
        </type>
        <type name="Cursor" restrictions="0">
            <field name="handle" restrictions="8"/>
            <method name="win32_new" restrictions="8" signature="(Lorg/eclipse/swt/graphics/Device;I)Lorg/eclipse/swt/graphics/Cursor;"/>
        </type>
        <type name="Device" restrictions="0">
            <method name="getDeviceZoom" restrictions="24" signature="()I"/>
            <method name="internal_dispose_GC" restrictions="8" signature="(JLorg/eclipse/swt/graphics/GCData;)V"/>
            <method name="internal_new_GC" restrictions="8" signature="(Lorg/eclipse/swt/graphics/GCData;)J"/>
        </type>
        <type name="Drawable" restrictions="0">
            <method name="internal_dispose_GC" restrictions="8" signature="(JLorg/eclipse/swt/graphics/GCData;)V"/>
            <method name="internal_new_GC" restrictions="8" signature="(Lorg/eclipse/swt/graphics/GCData;)J"/>
            <method name="isAutoScalable" restrictions="8" signature="()Z"/>
        </type>
        <type name="Font" restrictions="0">
            <field name="handle" restrictions="8"/>
            <method name="win32_new" restrictions="8" signature="(Lorg/eclipse/swt/graphics/Device;J)Lorg/eclipse/swt/graphics/Font;"/>
            <method name="win32_new" restrictions="8" signature="(Lorg/eclipse/swt/graphics/Device;JI)Lorg/eclipse/swt/graphics/Font;"/>
            <method name="win32_new" restrictions="8" signature="(Lorg/eclipse/swt/graphics/Device;Lorg/eclipse/swt/graphics/FontData;I)Lorg/eclipse/swt/graphics/Font;"/>
            <method name="win32_new" restrictions="8" signature="(Lorg/eclipse/swt/graphics/Font;I)Lorg/eclipse/swt/graphics/Font;"/>
        </type>
        <type name="FontData" restrictions="0">
            <field name="data" restrictions="8"/>
            <field name="height" restrictions="8"/>
            <method name="win32_new" restrictions="8" signature="(Lorg/eclipse/swt/internal/win32/LOGFONT;F)Lorg/eclipse/swt/graphics/FontData;"/>
        </type>
        <type name="FontMetrics" restrictions="0">
            <field name="handle" restrictions="8"/>
            <method name="win32_new" restrictions="8" signature="(Lorg/eclipse/swt/internal/win32/TEXTMETRIC;I)Lorg/eclipse/swt/graphics/FontMetrics;"/>
        </type>
        <type name="GC" restrictions="0">
            <method name="getGCData" restrictions="8" signature="()Lorg/eclipse/swt/graphics/GCData;"/>
            <field name="handle" restrictions="8"/>
            <method name="win32_new" restrictions="8" signature="(Lorg/eclipse/swt/graphics/Drawable;Lorg/eclipse/swt/graphics/GCData;)Lorg/eclipse/swt/graphics/GC;"/>
            <method name="win32_new" restrictions="8" signature="(JLorg/eclipse/swt/graphics/GCData;)Lorg/eclipse/swt/graphics/GC;"/>
        </type>
        <type name="GCData" restrictions="8"/>
        <type name="Image" restrictions="0">
            <field name="handle" restrictions="8"/>
            <method name="internal_dispose_GC" restrictions="8" signature="(JLorg/eclipse/swt/graphics/GCData;)V"/>
            <method name="internal_new_GC" restrictions="8" signature="(Lorg/eclipse/swt/graphics/GCData;)J"/>
            <field name="type" restrictions="8"/>
            <method name="win32_getHandle" restrictions="8" signature="(Lorg/eclipse/swt/graphics/Image;I)Ljava/lang/Long;"/>
            <method name="win32_new" restrictions="8" signature="(Lorg/eclipse/swt/graphics/Device;IJ)Lorg/eclipse/swt/graphics/Image;"/>
        </type>
        <type name="ImageData" restrictions="0">
            <method name="internal_new" restrictions="8" signature="(IIILorg/eclipse/swt/graphics/PaletteData;I[BI[B[BIIIIIII)Lorg/eclipse/swt/graphics/ImageData;"/>
        </type>
        <type name="Region" restrictions="0">
            <field name="handle" restrictions="8"/>
            <method name="win32_new" restrictions="8" signature="(Lorg/eclipse/swt/graphics/Device;I)Lorg/eclipse/swt/graphics/Region;"/>
        </type>
        <type name="TextLayout" restrictions="0">
            <method name="setDefaultTabWidth" restrictions="8" signature="(I)V"/>
        </type>
    </package>
    <package name="org.eclipse.swt.ole.win32" visibility="1">
        <type name="OleClientSite" restrictions="0">
            <method name="&lt;init&gt;" restrictions="8" signature="(Lorg/eclipse/swt/widgets/Composite;ILjava/lang/String;Ljava/io/File;)V"/>
        </type>
        <type name="OleControlSite" restrictions="0">
            <method name="&lt;init&gt;" restrictions="8" signature="(Lorg/eclipse/swt/widgets/Composite;ILjava/lang/String;Ljava/io/File;)V"/>
        </type>
        <type name="Variant" restrictions="0">
            <method name="win32_copy" restrictions="8" signature="(JLorg/eclipse/swt/ole/win32/Variant;)V"/>
            <method name="win32_new" restrictions="8" signature="(J)Lorg/eclipse/swt/ole/win32/Variant;"/>
        </type>
    </package>
    <package name="org.eclipse.swt.printing" visibility="1">
        <type name="PrintDialog" restrictions="2"/>
        <type name="Printer" restrictions="0">
            <field name="handle" restrictions="8"/>
            <method name="internal_dispose_GC" restrictions="8" signature="(JLorg/eclipse/swt/graphics/GCData;)V"/>
            <method name="internal_new_GC" restrictions="8" signature="(Lorg/eclipse/swt/graphics/GCData;)J"/>
            <method name="isAutoScalable" restrictions="8" signature="()Z"/>
        </type>
    </package>
    <package name="org.eclipse.swt.widgets" visibility="1">
        <type name="Button" restrictions="2"/>
        <type name="Caret" restrictions="2"/>
        <type name="ColorDialog" restrictions="2"/>
        <type name="Combo" restrictions="2"/>
        <type name="Control" restrictions="2">
            <field name="handle" restrictions="8"/>
            <method name="internal_dispose_GC" restrictions="8" signature="(JLorg/eclipse/swt/graphics/GCData;)V"/>
            <method name="internal_new_GC" restrictions="8" signature="(Lorg/eclipse/swt/graphics/GCData;)J"/>
        </type>
        <type name="CoolBar" restrictions="2"/>
        <type name="CoolItem" restrictions="2"/>
        <type name="DateTime" restrictions="2"/>
        <type name="Decorations" restrictions="2"/>
        <type name="DirectoryDialog" restrictions="2"/>
        <type name="Display" restrictions="2">
            <method name="findWidget" restrictions="8" signature="(J)Lorg/eclipse/swt/widgets/Widget;"/>
            <method name="findWidget" restrictions="8" signature="(JJ)Lorg/eclipse/swt/widgets/Widget;"/>
            <method name="findWidget" restrictions="8" signature="(Lorg/eclipse/swt/widgets/Widget;J)Lorg/eclipse/swt/widgets/Widget;"/>
            <method name="internal_dispose_GC" restrictions="8" signature="(JLorg/eclipse/swt/graphics/GCData;)V"/>
            <method name="internal_new_GC" restrictions="8" signature="(Lorg/eclipse/swt/graphics/GCData;)J"/>
            <field name="msg" restrictions="8"/>
            <method name="sendPostExternalEventDispatchEvent" restrictions="8" signature="()V"/>
            <method name="sendPreExternalEventDispatchEvent" restrictions="8" signature="()V"/>
        </type>
        <type name="ExpandBar" restrictions="2"/>
        <type name="ExpandItem" restrictions="2"/>
        <type name="FileDialog" restrictions="2"/>
        <type name="FontDialog" restrictions="2"/>
        <type name="Group" restrictions="2"/>
        <type name="IME" restrictions="2"/>
        <type name="Label" restrictions="2"/>
        <type name="Link" restrictions="2"/>
        <type name="List" restrictions="2"/>
        <type name="Menu" restrictions="2">
            <field name="handle" restrictions="8"/>
        </type>
        <type name="MenuItem" restrictions="2"/>
        <type name="MessageBox" restrictions="2"/>
        <type name="ProgressBar" restrictions="2"/>
        <type name="Sash" restrictions="2"/>
        <type name="Scale" restrictions="2"/>
        <type name="ScrollBar" restrictions="2"/>
        <type name="Scrollable" restrictions="2"/>
        <type name="Shell" restrictions="2">
            <method name="internal_new" restrictions="8" signature="(Lorg/eclipse/swt/widgets/Display;J)Lorg/eclipse/swt/widgets/Shell;"/>
            <method name="win32_new" restrictions="8" signature="(Lorg/eclipse/swt/widgets/Display;J)Lorg/eclipse/swt/widgets/Shell;"/>
        </type>
        <type name="Slider" restrictions="2"/>
        <type name="Spinner" restrictions="2"/>
        <type name="TabFolder" restrictions="2"/>
        <type name="TabItem" restrictions="2"/>
        <type name="Table" restrictions="2"/>
        <type name="TableColumn" restrictions="2"/>
        <type name="TableItem" restrictions="2"/>
        <type name="TaskBar" restrictions="2"/>
        <type name="TaskItem" restrictions="2"/>
        <type name="Text" restrictions="2"/>
        <type name="ToolBar" restrictions="2"/>
        <type name="ToolItem" restrictions="2"/>
        <type name="ToolTip" restrictions="2"/>
        <type name="Tracker" restrictions="2"/>
        <type name="Tray" restrictions="2"/>
        <type name="TrayItem" restrictions="2"/>
        <type name="Tree" restrictions="2"/>
        <type name="TreeColumn" restrictions="2"/>
        <type name="TreeItem" restrictions="2">
            <field name="handle" restrictions="8"/>
        </type>
        <type name="TypedListener" restrictions="14">
            <method name="&lt;init&gt;" restrictions="8" signature="(Lorg/eclipse/swt/internal/SWTEventListener;)V"/>
            <method name="getEventListener" restrictions="8" signature="()Lorg/eclipse/swt/internal/SWTEventListener;"/>
            <method name="handleEvent" restrictions="8" signature="(Lorg/eclipse/swt/widgets/Event;)V"/>
        </type>
        <type name="Widget" restrictions="0">
            <field name="nativeZoom" restrictions="8"/>
            <method name="removeListener" restrictions="24" signature="(ILorg/eclipse/swt/internal/SWTEventListener;)V"/>
            <method name="removeTypedListener" restrictions="24" signature="(ILjava/util/EventListener;)V"/>
        </type>
    </package>
</component>

So I see no descriptions for the Test files. I assume that's a good thing.

HeikoKlare · 2024-06-25T15:25:57Z

Sorry for being late in this discussion. I completely agree with everything written by @laeubi and @merks.

Even if not for SWT, it generally makes sense that project unit tests are not API.

This absolutely makes sense also from my point of view, and I think this is also the correct way of dealing with the SWT API errors. @merks, are your experiments in PDE worth to be provided as a PR? Otherwise I (as the one having introduced this kind of tests) could also try to find some time to have a look into an improvement of the API tools.

In my opinion, the tests are placed exactly where they need to be placed. I appreciate any pointers to alternative (technical) solutions, as currently I am not aware of any. The only "alternatives" I have seen so far (in platform code in general) is making things part of the bundle's API just for testing purposes (by providing packages or adding public methods only for testing) or by using reflection, but I consider none of them appropriate.

I'm not sure why the new tests are so different to every existing test that they have to be in production code.

Just for completeness: the difference of these tests in comparison to all existing tests is that they are the first unit tests (i.e., not integration or system tests) for win32-specific SWT code.

merks · 2024-06-25T15:31:38Z

@merks, are your experiments in PDE worth to be provided as a PR?

Definitely I will create a PR ASAP; I'm in Montreal until Thursday. I'll see what I can do quickly...

merks · 2024-06-25T15:56:50Z

I created the PR to improve/fix the API tools.

Are any of the changes proposed here appropriate or should this PR be closed?

HeikoKlare · 2024-06-25T16:39:45Z

Are any of the changes proposed here appropriate or should this PR be closed?

From my understanding, changes should be obsolete when API tooling behavior is improved. But probably @fedejeanne should respond/decide on that.

One additional thought: if I am not mistaken, using JUnit 5 instead of JUnit 4 would allow to reduce the test class visibility to package protected. This should also solve the API tools problem, but it would even avoid other potential tooling problems, which might lead to the test classes erroneously being considered as part of the API. And of course, keeping visibilities as low as possible is usually a good thing. So maybe that could be a follow-up task and a best practice for unit tests in production bundles?

merks · 2024-06-25T17:14:37Z

Is there a workaround for these that are public?

merks · 2024-06-25T19:07:44Z

Oh wait, package with the public base test classes is internal so those can be public without being API. If the tests that extend them are all package protected, there would be no API warnings as is now because those things are necessary not visible downstream:

That seems like a reasonable solution if the tests then still work.

HeikoKlare · 2024-06-25T20:49:38Z

If the tests that extend them are all package protected, there would be no API warnings as is now because those things are necessary not visible downstream:

Exactly. It should be possible make each class either package protected or have it placed in a package that is not part of public API anyway. From a conceptual point of view, I do not see any other potential visibility-related constraints.

Note that any potential restriction due to subclassing is something that can and should usually be avoided, as often there is no need for subclassing in tests (in particular when using JUnit 5) and there are very good reasons to avoid inheritance when writing tests in general (since JUnit 3 times are long over). I remember disentangling the ResourceTest base class in Eclipse platform, which was quite awful. Mostly subclassing is only used as a bad way of reuse, compared to static utility or helper classes and, in particular, the usage of well-defined JUnit extensions with single, dedicated responsibilities.

merks · 2024-06-25T20:55:52Z

@HeikoKlare

I will hold off on merging the API tools changes until we establish there is no better way to simply avoid the problem via class visibility and/or internal packages. There's no point in complicating those tools or adding performance overhead to those tools if the problem can simply be avoided entirely.

Thanks for your constructive suggestions!

merks · 2024-06-25T21:57:15Z

Maybe it should be fixed like this?

#1303

HeikoKlare · 2024-06-26T06:53:06Z

Maybe it should be fixed like this?

#1303

Unfortunately, only reducing test class visibility but sticking to JUnit 4 does not seem to be possible (see #1303 (review)).

Let's test a migration of the tests to JUnit 5: #1305

vogella · 2024-06-26T07:18:07Z

Maybe you can try removing the exported packages from the MANIFEST.MF. Tests are not API (PMC decision a while ago), therefore would be OK.

Unless of course other tests need the exports.

fedejeanne · 2024-06-26T08:57:45Z

Better go with #1305

fedejeanne requested a review from niraj-modi as a code owner June 24, 2024 06:56

fedejeanne force-pushed the fix_api_errors_swt_tests branch from 57e98b0 to f2ec94f Compare June 24, 2024 06:57

fedejeanne marked this pull request as draft June 24, 2024 06:58

laeubi reviewed Jun 24, 2024

View reviewed changes

binaries/org.eclipse.swt.win32.win32.x86_64/META-INF/MANIFEST.MF Outdated Show resolved Hide resolved

fedejeanne force-pushed the fix_api_errors_swt_tests branch from f2ec94f to 57b745f Compare June 24, 2024 07:03

fedejeanne marked this pull request as ready for review June 24, 2024 07:03

Fix API errors in SWT due to tests eclipse-platform#1298

cb7188b

- Move Win32AutoscaleTestBase to package org.eclipse.swt - Increase minor version in manifest - Add missing @SInCE tags in extending test classes Fixes eclipse-platform#1298

fedejeanne force-pushed the fix_api_errors_swt_tests branch from 57b745f to cb7188b Compare June 24, 2024 07:09

iloveeclipse requested changes Jun 24, 2024

View reviewed changes

fedejeanne mentioned this pull request Jun 24, 2024

Unit test writing for controls to test HiDpi #1292

Merged

2 tasks

HeikoKlare mentioned this pull request Jun 25, 2024

SWT tests for win32 have API errors #1298

Closed

fedejeanne closed this Jun 26, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix API errors in SWT due to tests #1299

Fix API errors in SWT due to tests #1299

fedejeanne commented Jun 24, 2024 •

edited

Loading

github-actions bot commented Jun 24, 2024 •

edited

Loading

iloveeclipse left a comment

fedejeanne commented Jun 24, 2024

iloveeclipse commented Jun 24, 2024

merks commented Jun 24, 2024

iloveeclipse commented Jun 24, 2024

iloveeclipse commented Jun 24, 2024

deepika-u commented Jun 24, 2024 •

edited

Loading

fedejeanne commented Jun 24, 2024

iloveeclipse commented Jun 24, 2024

laeubi commented Jun 24, 2024 •

edited

Loading

laeubi commented Jun 24, 2024

iloveeclipse commented Jun 24, 2024

fedejeanne commented Jun 24, 2024

laeubi commented Jun 24, 2024 •

edited

Loading

merks commented Jun 25, 2024

laeubi commented Jun 25, 2024

merks commented Jun 25, 2024

merks commented Jun 25, 2024 •

edited

Loading

HeikoKlare commented Jun 25, 2024

merks commented Jun 25, 2024

merks commented Jun 25, 2024

HeikoKlare commented Jun 25, 2024

merks commented Jun 25, 2024

merks commented Jun 25, 2024

HeikoKlare commented Jun 25, 2024

merks commented Jun 25, 2024

merks commented Jun 25, 2024

HeikoKlare commented Jun 26, 2024

vogella commented Jun 26, 2024

fedejeanne commented Jun 26, 2024

Fix API errors in SWT due to tests #1299

Fix API errors in SWT due to tests #1299

Conversation

fedejeanne commented Jun 24, 2024 • edited Loading

github-actions bot commented Jun 24, 2024 • edited Loading

Test Results

iloveeclipse left a comment

Choose a reason for hiding this comment

fedejeanne commented Jun 24, 2024

iloveeclipse commented Jun 24, 2024

merks commented Jun 24, 2024

iloveeclipse commented Jun 24, 2024

iloveeclipse commented Jun 24, 2024

deepika-u commented Jun 24, 2024 • edited Loading

fedejeanne commented Jun 24, 2024

iloveeclipse commented Jun 24, 2024

laeubi commented Jun 24, 2024 • edited Loading

laeubi commented Jun 24, 2024

iloveeclipse commented Jun 24, 2024

fedejeanne commented Jun 24, 2024

laeubi commented Jun 24, 2024 • edited Loading

merks commented Jun 25, 2024

laeubi commented Jun 25, 2024

merks commented Jun 25, 2024

merks commented Jun 25, 2024 • edited Loading

HeikoKlare commented Jun 25, 2024

merks commented Jun 25, 2024

merks commented Jun 25, 2024

HeikoKlare commented Jun 25, 2024

merks commented Jun 25, 2024

merks commented Jun 25, 2024

HeikoKlare commented Jun 25, 2024

merks commented Jun 25, 2024

merks commented Jun 25, 2024

HeikoKlare commented Jun 26, 2024

vogella commented Jun 26, 2024

fedejeanne commented Jun 26, 2024

fedejeanne commented Jun 24, 2024 •

edited

Loading

github-actions bot commented Jun 24, 2024 •

edited

Loading

deepika-u commented Jun 24, 2024 •

edited

Loading

laeubi commented Jun 24, 2024 •

edited

Loading

laeubi commented Jun 24, 2024 •

edited

Loading

merks commented Jun 25, 2024 •

edited

Loading