Save screenshots to Maven build output directory#3819
Conversation
basilevs
force-pushed
the
surefire_screenshot
branch
from
0b25779 to
c5aabc8
Compare
|
@merks @HeikoKlare could you suggest a better heuristic or another way to work with CI screenshots in SWT? |
|
I have no knowledge in this area. Maybe @HannesWell has a good idea. |
|
It sounds reasonable to have a properly defined place for storing the screenshots so that they can be archived during GHA runs. I remember that I did some tweaks when analyzing an issue only occurring in GHA runs for which I required screenshots. So having a general solution for this would be great. Thank you for putting effort into this. I wonder if we couldn't just define a proper value for the existing system property Regarind the proposed change: Unless the Eclipse platform is present and the system property for the output folder is specified, the results will now always be written into the surefire target repository (if existing). Is that what we want to happen? For example, when executing the tests locally in the IDE, do we want to have the screenshots placed in a Maven-build-specific folder? In case I have execute a local Maven build at some earlier point in time, that folder will be present and used. Maybe @akurtakov can also help here, as he guarded the screenshot output folder extraction with the Platform being present recently: |
Is not it an application argument and not a system property? |
You're right. Then we maybe want to check for a GHA environment and then define a dedicated folder that is also used for artifact upload? |
basilevs
force-pushed
the
surefire_screenshot
branch
from
c5aabc8 to
c9bbda8
Compare
The decision should be based on whether Maven is in use, not on which environment it is running on. As some projects reuse aggregator's workflow, and most use it's parent POM, the maintenance effort per-project should be minimal. |
basilevs
changed the title
HannesWell
left a comment
HannesWell
left a comment
There was a problem hiding this comment.
Generally I think this is fine.
Do you know if the screenshots are also saved for I-build tests and if some according adaption is needed then? I'm not aware of it, but might have missed it.
| <printApiMessages>false</printApiMessages> | ||
| <failOnJavadocErrors>false</failOnJavadocErrors> | ||
| <compileLogDir>${project.build.directory}/compilelogs</compileLogDir> | ||
| <eclipse.screenshot_directory>${project.build.directory}/screenshots</eclipse.screenshot_directory> |
There was a problem hiding this comment.
Wouldn't the property be better named surefire.screenshot_directory (and be placed next to the other surefire related properties)?
| <eclipse.screenshot_directory>${project.build.directory}/screenshots</eclipse.screenshot_directory> | |
| <surefire.screenshot_directory>${project.build.directory}/screenshots</surefire.screenshot_directory> |
There was a problem hiding this comment.
The JVM system property is naturally usable outside of surefire. For example, it could be configured via launch configuration. This makes naming it surefire specific confusing.
Maven property was named to exactly match the JVM system property to help with identification and search.
I did fix screenshots for platform.ui a few years back. Those relied on |
basilevs
force-pushed
the
surefire_screenshot
branch
from
c9bbda8 to
1e5e3cc
Compare
|
In general, the approach also looks fine for me now. You mention this older PR that states to ensure that test screenshots are provided in the GH workflow artifacts: That one did not affect I-Builds but just GH workflows and introduced archiving of screenshots placed here:
I ran local Maven builds and explicitly made some tests fail. The produced screenshots are placed in the temp folder. So I wonder if this configuration is still valid? Also note that there is further documentation (and outdated code relying on Gerrit) in the |
I can't find how Ant tests (which store data in
Do you suggest to cleanup Gerrit code as part of this unrelated feature? While I can certainly do so, I have no means to reliably test it (without committer level access to all involved CI infra) and would like to avoid mixing concerns.
I see no indication that |
Why do you assume Ant tests to store data in "result/*"? From what I see, the configuration is only used in a GH workflow, so used by GHA runs. The GHA runs use Maven to execute tests and the Maven execution does not seem to place the screenshots at any reasonable place currently. The Ant tests set the
I just suggest to follow boy scout rule and clean up the current implementation of how the path for storing the screenshots is defined instead of adding further cases at a "random" position. In addition:
The method is about processing the That's why I propose to streamline the implementation by removing what is obsolete and cleaning up what needs to remaind or is added:
If you want, I can also provide a preparatory PR removing the obsolete code/configuration so that you can then add your new configuration on top. |
The argument is named this way, because it manages JUnit output. Arguably, method name matches the argument, because they serve the same purpose, not because method implementation depends on argument. It does not matter though, I'll move stuff around to unblock.
I will apply these changes as requested, but am unable to test that.
I'll do the change, but it would be nice to somehow run all related CI's on it. I'm not sure if it even possible (given all sub-projects have their own configurations, some reusing |
basilevs
force-pushed
the
surefire_screenshot
branch
2 times, most recently
from
a197d37 to
e2aaa3d
Compare
|
@HeikoKlare I've applied all suggestions.
When I was doing the old PR, I assumed that Maven somehow started Ant tests for platform.ui project. This does not seem to be the case anymore, but I can't verify that. A few of recent failed builds in platform.ui do not produce screenshots at all (no message about AWT screenshot in console). Do you happen to know what is going on there? BTW:
Surprisingly not a lot of them. Still, we can't be sure, that projects out of aggregator would not break. |
HeikoKlare
left a comment
HeikoKlare
left a comment
There was a problem hiding this comment.
I've applied all suggestions.
Thank you! It looks fine to me and I am sure this will help us in the future to better understand the code. @HannesWell what do you think about the changes?
When I was doing the old PR, I assumed that Maven somehow started Ant tests for platform.ui project. This does not seem to be the case anymore, but I can't verify that. A few of recent failed builds in platform.ui do not produce screenshots at all (no message about AWT screenshot in console). Do you happen to know what is going on there?
I have researched in history for a while now, trying to understand why/how this could have ever worked (using the "target/results" directory), but I have no clue yet. To the best of my knowledge, the GH workflows always used pure Maven (with surefire for test execution) and Ant was only used for I-Build tests. Maybe there was a different configuration at some point in time (in particular the Screenshots class was heavily modified and moved around) that made this work. Are you sure that the artifacts upload has ever worked?
I also wondered where screenshots are actually taken. I assumed that the ScreenshotOnFailure/Screenshots.onFailure() rules is used by many UI tests in Platform UI, but that's not the case. Some usages have silently been removed during JUnit 5 migration, such as the one in PartRenderingEngineTests via eclipse-platform/eclipse.platform.ui#3636.
@vogella do you know if there were more usages of this JUnit 4 rule that have been removed during JUnit 5 migration?
BTW: Following aggregator subprojects projects use
buildMaven.yml:
- eclipse.pde
- eclipse.platform.ui
- eclipse.platform
- equinox.p2
Surprisingly not a lot of them. Still, we can't be sure, that projects out of aggregator would not break.
Projects out of aggregator are not supposed to use those workflows. But anyway, where do you see a chance that anything could break? The change in the workflow descriptions just affects an included folder in artifacts upload, so the worst I can expect is that some PNGs are not uploaded as expected?
| @Deprecated | ||
| public static boolean isRunByGerritHudsonJob() { |
There was a problem hiding this comment.
This plugin is not public API, so we don't need to deprecate this but can just remove it.
There was a problem hiding this comment.
For non-public API, it is used a lot:
There was a problem hiding this comment.
Sorry, my comment was a bit imprecise: the plugin is not part of the SDK but only used internally for tests inside the SDK. And within the SDK, there are no further consumers of this method:
eclipse-platform/eclipse.platform.swt#3303 As screenshots are effectively a build artifact, save them in build output directory (target/ by default). SWT does not use Platform, so existing heuristics have not worked in GitHub Actions, abandoning screenshots in /tmp without archiving into artifacts. The change breaks existing Gerrit jobs by removing public method `Screenshots.isRunByGerritHudsonJob()`, but as Gerrit is no longer used, this should be fine.
basilevs
force-pushed
the
surefire_screenshot
branch
from
e2aaa3d to
c897f17
Compare
Well, given this is a second time I'm fixing these screenshots archival I would not wish for anyone to do this yet again. |
I have to admit that I still don't get which risk we need to consider here. The archival is already broken (and probably never worked), so what do you think might become worse with this PR? |
4 participants
Partial fix for eclipse-platform/eclipse.platform.swt#3303.
SWT does not use Platform, so existing heuristics do not work in GitHub Actions.
Once this more sensible screenshot location is set, SWT GHA workflows can be updated to archive screenshots to artifacts. Before the change they have been thrown away and could not be inspected.