-
Notifications
You must be signed in to change notification settings - Fork 188
Skip build and test on OSes that have no changes #2764
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Skip build and test on OSes that have no changes #2764
Conversation
bbc0b9d to
73e542b
Compare
This comment was marked as outdated.
This comment was marked as outdated.
|
I totally like the idea of skipping tests for OSes that are not affected by a change. I am just a bit concerned about the condensed test results posted by the bot being rendered less useful. Maybe we could check if we can adapt that the results to be separated by OS, such that you can easily and properly compare for the OS(es) of interest for the current PR. Edit: just saw that Christoph already made a similar comment: #2762 (comment) |
|
I am going to split this PR out from the rest of them for #2741 |
As mentioned on the other PR (sorry for confusion!) I think we then better should have three check runs for all three oss to prevent the problem you see with the test drops and then have one comment for each os e.g.
beside that I'm wondering if it really pay off much to skip the tests, the Jenkins if often the slowest one and Github has completed most often much earlier anyways. Also I'm wondering what happens with changes to the common parts of SWT that can change behavior on other OS of course as well. |
From my understanding, there is a classification for those changes that enables all OS builds via
Even though this is often the case, in my experience "often" rather means 60% of the cases than 90%, so avoiding unnecessary GHA runs will my in opinion still increase the average roundtrip time. And I see at least two further reasons to do this:
|
|
@laeubi / @HeikoKlare thanks for your input - I agree and will see if I have time to bring this to a conclusion.
Yes, that is correct, anything that isn't in the platform specific changes (e.g |
As part of working on eclipse-platform#2764 I was considering how to display badges going forward to align with test results per platform. As it turns out the "SWT Matrix Tests" has been stuck on an arbitrary old badge for many years now. This PR updates it, but as it has been wrong for a long time, perhaps removing it entirely makes more sense, but for now I update it. There was push back in eclipse-platform#1897 against removing it. Part of eclipse-platform#2714
As part of working on #2764 I was considering how to display badges going forward to align with test results per platform. As it turns out the "SWT Matrix Tests" has been stuck on an arbitrary old badge for many years now. This PR updates it, but as it has been wrong for a long time, perhaps removing it entirely makes more sense, but for now I update it. There was push back in #1897 against removing it. Part of #2714
This badge has been incorrect since its inception and keeping it maintained properly going forward would require some additional work in the context of eclipse-platform#2764 Therefore, this removes the badge. Fixes eclipse-platform#2783 Part of eclipse-platform#2714
73e542b to
2c56a05
Compare
Many of the PRs on SWT are OS specific, this change only runs the build on OSes if there are changes that can affect that OS. This is going to be more important with eclipse-platform#2714 because potentially a number of new jobs will be run per PR and avoiding some of the extra time waiting will be a benefit to all. The junit.yml has been updated to record tests per platform in addition to all tests. When not all platforms are run in a PR, only the respective platform results are reported as a comment. If all platforms are run, the cumulative results are reported as a comment. Part of eclipse-platform#2714
2c56a05 to
df9e5bb
Compare
|
I have now implemented test result reporting based on what has run. You can see what it will look like once this change is merged in the 4 examples I provide in my fork: Single platform onlyOnly the platform that was run generates a comment.
Common code (or any change that triggers all the platforms)Nothing is skipped in this case. Since linux + macos are matrix jobs, the total number of matrix jobs run is 6 (will be more once #2768 is merged). The comment added to the PR is the summary of all the jobs.
If you click through the results you can also see the results of the individual platforms. This is especially useful when reviewing skipped jobs and a few of the other annotations.
2 platforms changeIn this case the 3rd platform SWT Matrix job is skipped. This means that two comments are created, for the two platforms that did run.
Only markdown changed (or anything in a category with no associated jobs)In this case all the SWT Matrix jobs are skipped and no test results are reported.
|
Actually 5 - I realized I missed an important case, what failures look like. How errors reportIn this example PR I have a ridiculous failures that throws an exception in the Display constructor meaning almost everything fails. This is what that looks like.
Test report diff to base commitWhen I made the screenshots in #2764 (comment) the base commit had not finished running yet, so they were not showing diffs to base commit. I updated all the linked example PRs, but I am only redoing this screenshot:
|
|
This is now ready for review. |







Many of the PRs on SWT are OS specific, this change only runs the build on OSes if there are changes that can affect that OS.
This is going to be more important with #2714 because potentially a number of new jobs will be run per PR and avoiding some of the extra time waiting will be a benefit to all.
The junit.yml has been updated to record tests per platform in addition to all tests. When not all platforms are run in a PR, only the respective platform results are reported as a comment. If all platforms are run, the cumulative results are reported as a comment.
Part of #2714