Reduce Win32 FontRegistries visibility and move tests inside fragment

[HeikoKlare]

The classes DefaultSWTFontRegistry and ScalingSWTFontRegistry are currently public only to make them testable from a test bundle. There is currently no pre-configured way to implement unit tests for classes that are not directly accessible via public API.

With this change, the SWT Win32 fragments are extended by a configuration for providing fragment-internal unit tests executed with plain Maven surefire, which can access the classes under test even with reduced visibility. Due to current limitations in Tycho (see eclipse-tycho/tycho#3803), an according temporary extension to the Maven configuration for the binaries projects is made. This configuration is applied to the tests for the DefaultSWTFontRegistry and ScalingSWTFontRegistry, such that their visibilities can properly be reduced to package visibility.

[github-actions]

Test Results

   418 files  +16     418 suites  +16   7m 46s ⏱️ +52s
 4 121 tests ± 0   4 113 ✅ ± 0   8 💤 ±0  0 ❌ ±0 
16 313 runs  ± 0  16 221 ✅ ± 0  92 💤 ±0  0 ❌ ±0 

Results for commit 0c56201. ± Comparison against base commit b58f379.

♻️ This comment has been updated with latest results.

[laeubi]

Test folders should usually not be part of the source.xxx / bin.xxx of PDE...

[HeikoKlare]

I think I am not yet sure how to properly make Tycho compile the test classes. With this configuration, Tycho properly generates test classes in a separate output folder to be used for executing the maven-surefire-plugin. What would be the proper approach to achieve that?

[laeubi]

it will jsut work without that, as it is configured in the .classpath already.

[HeikoKlare]

That's correct. Only the .classpath configuration is currently not considered because the .classpath file is not considered due to being a linked resource.

[laeubi]

For test folder use a JUNIT Classpath container instead of additional bundles (what server a completely different purpose)

[HeikoKlare]

I wanted to ask you exactly what you think is the proper approach here, as additional.bundles were only my first attempt to get things running. I have added this as a means to make Tycho have the JUnit dependencies at compile-/test-time. I've already specified a JUnit classpath container in the .classpath, but that's not considered for the Tycho build, is it? At least Tycho complains about missing JUnit dependencies when compiling the test sources. Probably it would be better to add the JUnit dependency directly to the Maven build?

[laeubi]

I've already specified a JUnit classpath container in the .classpath, but that's not considered for the Tycho build, is it?

This is supported since Tycho 2.4

At least Tycho complains about missing JUnit dependencies

Maybe a bug or misconfiguration, at least we have some test for this here that should prove it works.

Probably it would be better to add the JUnit dependency directly to the Maven build?

While it maybe works, the purpose of Tycho is to not need special maven configuration so it might be a good opportunity to find out why it does not work and either improve Tycho or the documentation (sadly it misses the part about eclipse setup currently).

[HeikoKlare]

Then something does obviously not work here as expected:

• Tycho does not consider the test source folder specified via the .classpath. If I understand correctly, it should automatically use those test source folder to compile test classes.
• When specifying the test source and output folder manually in the build.properties, Tycho complain about missing JUnit dependencies when compiling them.

I guess that both the specification of test source and output folder in the build.properites as well as the additional specification of JUnit dependencies relate to the same root cause, due to which the information from the .classpath file are not considered correctly. I will try to figure out what is special in the SWT setup compared to the Tycho integration tests for that behavior.

[laeubi]

it might be because classpath is a linked file, can you try to copy the file as a regular one into the project? I recently added support for linked files, but that for sure not respected everywhere at the moment.

[HeikoKlare]

Thank you, @laeubi! The linked files are actually the problem here. It affects both the linked .classpath file as well as the linked test source folder.

• When replacing the linked .classpath with the same file directly placed in the project, Tycho properly considers the test source folder specified in the .classpath file. Then it only complains that it does not find that (linked) folder:
  [WARNING] Source directory C:\dev\eclipse\platform-only\git\eclipse.platform.swt\binaries\org.eclipse.swt.win32.win32.x86_64\Eclipse SWT Tests\win32 does not exist
• When also replacing the linked source test folder with the same folder directly placed in the project, test source files are compiled correctly and tests are executed.

[laeubi]

okay then this needs some adjustment in Tycho first, I'll try to take a look. As always if one want to
provide an integration-test to demonstrate the issue that would be helpful.

[HeikoKlare]

I have already created an according integration test (on https://github.com/HeikoKlare/tycho/tree/issue_3803_reproducer) and will provide a describing issue and a PR with that test to the Tycho project tomorrow.

[HeikoKlare]

Tycho issue is now documented along with a reproducer PR: eclipse-tycho/tycho#3803

[HeikoKlare]

I have restructured the custom configuration for running the tests via Tycho as a temporary solution until eclipse-tycho/tycho#3803 has been addressed. The solution has the following main properties to make it easy to replace with an in-build Tycho solution once available:

• Makes the Maven build behave exactly as if the Tycho issue was resolved (i.e., as if .classpath and the test source folder were no linked resources)
• Only affects Maven configuration (no PDE configuration, such as with source and output specification or additional.bundles in the build.properties)

I consists of the following additions:

• An explicit JUnit dependency in the pom.xml of the binaries folder, I preferred this over directly adding the configuration to a pom.xml in the win32 fragments, as they have auto-generated POMs which I do not want to replace with fixed POMs just for a temporary solution.
  An active-by-default profile to the pom.xml of the binaries folder, which adds the necessary JUnit dependencies. I preferred the profile over directly adding the configuration to a pom.xml in the win32 fragments, as they have auto-generated POMs which I do not want to replace with fixed POMs just for a temporary solution.
• Configures the testSourceDirectory by making it configurable via a Maven property that is set via the win32 fragments' build.properties.

I have added comments to all additions that are part of the temporary solution to easily remove them later on.

@laeubi What do you think about this temporary solution? Since the colleagues working on Windows HiDPI support have a rather urgent demand for being able to provide this kind of tests, I would like to integrate this (or some other kind of temporary solution) to not depend on when the Tycho feature can/will be realized.

For whomever is wondering that it might make sense to directly start with JUnit 5 instead of JUnit 4: I want to keep this initial restructuring PR as simple as possible (and the moved tests are JUnit 4) and will provide a follow-up that migrates to JUnit 5.

[laeubi]

@HeikoKlare if it works I won't object in general, but maybe it also works for your college to simply not branch from master but from your brach and add items on top of it?

Then it should work to already develop / test things and later on only needs a rebase on master if we (hopefully soon) have the final solution.

[HeikoKlare]

if it works I won't object in general, but maybe it also works for your college to simply not branch from master but from your brach and add items on top of it?

In general, I would prefer that over having an intermediate solution in master, However, the tests come together with several functional additions. These functional additions depend on each other and we want / need to merge them into master in small increments to ensure continuous validation and minimize the risk of regressions. Merging all of them in a separate branch would lead to a large merge into master afterwards with a too high risk of producing regressions.

[laeubi]

I somehow remmber we already have a way in SWT to determine the platform.

[merks]

org.eclipse.swt.tests.junit.SwtTestUtil.isWindows

[HeikoKlare]

Yes, there are at least two ways to determine the platform, but unfortunately none of them is applicable here:

1. The platform for which the currently accessed fragment as stored in Platform.PLATFORM. At usual runtime, you have the fragment matching your OS loaded (as defined by the platform filter of the fragment), so you can use this to determine the platform you are running. When executing these tests, however, you will always access the Platform class from the fragment that you are currently processing, so Platform.PLATFORM will always be "win32" in this case, even if you are running on Linux or MacOS.
2. The SwtTestUtil (as referred to by Ed). But they are placed in the downstream test plug-in and not in the SWT plug-in itself.

But since I have moved this util class to the same package as the Library class to retrieve the system architecture from the Library class, I can now also retrieve the OS from that class (which does the same as the SwtTestUtil). I have adapted this method accordingly in 91a97ca.

[HeikoKlare]

Thanks to @laeubi's fast improvement for Tycho, the tests inside the SWT fragments now run properly without custom Maven configuration. I was able to update / simplify this PR accordingly in b256ce7. So we can merge this without adding any temporary solution.

The only thing remaining is that the test source folder linked in the project description cannot use the SWT_HOST_PLUGIN variable, as Tycho does not support custom variables yet: eclipse-tycho/tycho#3820
I consider this acceptable and something that can be improved if/once Tycho supports variables.

[HannesWell]

There is currently no pre-configured way to implement unit tests for classes that are not directly accessible via public API.

Just as a quick remark without knowing if this has already been considered: The 'PDE-way' to achieve this is to make the test a fragment of the tested plugin and place the test in the same package. Or is this not possible due to the special structure of the SWT bundles/fragments?

[laeubi]

Or is this not possible due to the special structure of the SWT bundles/fragments?

A fragment can't have another fragment as a host.

[HannesWell]

Or is this not possible due to the special structure of the SWT bundles/fragments?

A fragment can't have another fragment as a host.

Of course, but it could just use org.eclipse.swt just like it requires it now.
But any way, I just wanted to make sure all options were on the table. In general it is not bad to have the tests in the Plug-in project (like it is common in Maven projects), I think it would just be good to have all (windows-)tests at one place.

[HeikoKlare]

But any way, I just wanted to make sure all options were on the table. In general it is not bad to have the tests in the Plug-in project (like it is common in Maven projects),

We have discussed different options on how to realize such tests in a separate discussion thread (eclipse-platform/.github#203). Seems like I forgot to reference the discussion in this PR.

I think it would just be good to have all (windows-)tests at one place.

The Windows tests (as well as potentially other OS tests or even common tests) can/will be at two places:

1. The (common or OS-specific) test plug-in, testing the SWT plug-in's API (already existing now)
2. "Internal" tests for abstraction layers within the plug-in/fragment that require testing but are not part of public API (like the ones in this PR).

[HannesWell]

But any way, I just wanted to make sure all options were on the table. In general it is not bad to have the tests in the Plug-in project (like it is common in Maven projects),

We have discussed different options on how to realize such tests in a separate discussion thread (eclipse-platform/.github#203). Seems like I forgot to reference the discussion in this PR.

Sorry I have missed that (I'm not subscribed for all events in the platform and platform.ui repo).
Thank you for your elaboration.

[laeubi]

Of course, but it could just use org.eclipse.swt just like it requires it now.

While this technically works, I'm not sure the tooling (IDE + Build) is really prepared for that, lets assume we Have Host, Fragement and Test, where Fragment and Test are bot fragments to the Host, to allow Test to access Fragment classes we must make sure they are always used together and there is some way for Test to require Fragment + Host so the compiler can see the classes.

[HeikoKlare]

Of course, but it could just use org.eclipse.swt just like it requires it now.

While this technically works, I'm not sure the tooling (IDE + Build) is really prepared for that, lets assume we Have Host, Fragement and Test, where Fragment and Test are bot fragments to the Host, to allow Test to access Fragment classes we must make sure they are always used together and there is some way for Test to require Fragment + Host so the compiler can see the classes.

I tried this as an alternative to this PR with the initial, temporary solution (when Tycho support was linked .classpath files was missing) but did not find a proper way to enable the test fragment to access classes of the Windows fragment. I did not invest too much time but it did not feel as good as the solution of having the unit tests directly within the fragments anyway.