Skip to content
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

Enable PerformanceTests #882 #906

Merged

Conversation

basilevs
Copy link
Contributor

@basilevs basilevs commented Nov 27, 2023

Enable PerformanceTests for Pull Requests labeled "performance".
See discussion on why tests can't be enabled on a permanent basis.

@laeubi
Copy link
Contributor

laeubi commented Nov 27, 2023

@basilevs just in case you are interested, I could think about triggering a workflow by adding a label to a PR that then additionally executes the performance tests on demand.

@basilevs
Copy link
Contributor Author

@laeubi that would be very nice indeed. I suspect this PR can't be used as test ground anyway because by default Github does not use workflows from unverified PRs.

@basilevs basilevs changed the title Enable PerofrmanceTests #882 Enable PerformanceTests #882 Nov 27, 2023
@laeubi
Copy link
Contributor

laeubi commented Nov 27, 2023

Yes one need to develop such workflow on the a fork as it requires to merge things to master (can be reset later on of course) and have additional permissions, but as you seem very enthusiastic on this maybe you like to go in that direction as well 👍

@laeubi
Copy link
Contributor

laeubi commented Nov 27, 2023

Even though I approved the Run it seems the Github actions failed to execute, do you like to take a look?

@basilevs
Copy link
Contributor Author

I've missed a colon in step declaration. Would be nice to get an error from that.

@laeubi
Copy link
Contributor

laeubi commented Nov 27, 2023

I've missed a colon in step declaration. Would be nice to get an error from that.

yes that a bit unfortunate but you should receive an email if there is a fatal error in the action, also you can edit the file with github UI and then gets a validation.

Copy link
Contributor

github-actions bot commented Nov 27, 2023

Test Results

     302 files  +  3       302 suites  +3   17m 2s ⏱️ + 11m 28s
  4 102 tests +  7    4 094 ✔️ +  7    8 💤 ±0  0 ±0 
12 218 runs  +21  12 144 ✔️ +20  74 💤 +1  0 ±0 

Results for commit 158e4c6. ± Comparison against base commit f677076.

♻️ This comment has been updated with latest results.

@basilevs
Copy link
Contributor Author

basilevs commented Nov 27, 2023

Yes one need to develop such workflow on the a fork as it requires to merge things to master (can be reset later on of course) and have additional permissions, but as you seem very enthusiastic on this maybe you like to go in that direction as well 👍

@laeubi could you please mark my pull requests with performance label?

@basilevs basilevs force-pushed the issue_882_performanceTestOnCI branch from 95a4811 to c288a29 Compare November 27, 2023 11:24
@laeubi laeubi added the performance Performance issue label Nov 27, 2023
@basilevs basilevs force-pushed the issue_882_performanceTestOnCI branch from c288a29 to c5aa813 Compare November 27, 2023 11:28
@laeubi
Copy link
Contributor

laeubi commented Nov 27, 2023

@basilevs have you tested this with your fork? I think even if I aprove the workflow run your changes are not picked up maybe because you are not a cmoitter on the repo.

@basilevs
Copy link
Contributor Author

basilevs commented Nov 27, 2023

@basilevs have you tested this with your fork?

Here is my test build

I'm still not sure it it breaks artifact collection. And something strange is going on with MacOS.

@basilevs
Copy link
Contributor Author

basilevs commented Nov 27, 2023

API Tools fail the build. What is the workflow here? Last time I've tried to bump versions, it did not end well.

On a positive note I see my additional step in the action logs.

Uploading Screenshot 2023-11-27 at 15.47.19.png…

@iloveeclipse
Copy link
Member

[ERROR] Failed to execute goal org.eclipse.tycho.extras:tycho-p2-extras-plugin:4.0.4:compare-version-with-baselines (compare-attached-artifacts-with-release) on project org.eclipse.swt: Only qualifier changed for (org.eclipse.swt/3.124.200.v20231127-1131). Expected to have bigger x.y.z than what is available in baseline (3.124.200.v20231113-1355)

This means, this is first change in the release in this bundle but manifest version is not updated.
Depending on the change you have to bump either last version segment (if the API os not changed) or second one (if API is affected).

@basilevs
Copy link
Contributor Author

basilevs commented Nov 27, 2023

[ERROR] Failed to execute goal org.eclipse.tycho.extras:tycho-p2-extras-plugin:4.0.4:compare-version-with-baselines (compare-attached-artifacts-with-release) on project org.eclipse.swt: Only qualifier changed for (org.eclipse.swt/3.124.200.v20231127-1131). Expected to have bigger x.y.z than what is available in baseline (3.124.200.v20231113-1355)

This means, this is first change in the release in this bundle but manifest version is not updated. Depending on the change you have to bump either last version segment (if the API os not changed) or second one (if API is affected).

No code was changed (the change modifies only .github directory).
I've created #909 to bump the version

@basilevs basilevs force-pushed the issue_882_performanceTestOnCI branch from c5aa813 to 474ab5e Compare November 27, 2023 13:33
@basilevs basilevs marked this pull request as ready for review November 27, 2023 13:53
@basilevs
Copy link
Contributor Author

basilevs commented Nov 27, 2023

This works, but one test takes a a lot of time on MacOS. Looks like a performance bug. I've created #912

@basilevs
Copy link
Contributor Author

@akurtakov I see that performance test assertions were disabled three years ago. Now they can never fail. Should this be fixed? I assume lots of tests are using these assertions in vain now.

eclipse-platform/eclipse.platform.releng@071af51#diff-55cfefb6d6ca7ae136d8057fc7ea93d8b62af5db44fc77cec0f961fc68d2bdbdL85-L107

@akurtakov
Copy link
Member

@basilevs 3 years ago perf tests have been migrated from storing both (current and latest release) timings in a db (this database server was a nightmare to maintain and historical data with OS updates and etc. proved to be useless) to using the test files which contain timings to show the diffs in perfermance . This also degraded over time:

Long story short perf tests were running (minus infra/releng failures) till March this year . The way these perf tests worked was to run the same test twice - once with previous release and once with latest I-build and compare timings. The disabled assertions were actually failing based on "timing" criteria from DB content which as said previously is not that useful with fast changing environments nowadays.

@basilevs basilevs force-pushed the issue_882_performanceTestOnCI branch 2 times, most recently from d7f2150 to 11bd208 Compare November 29, 2023 14:13
@basilevs
Copy link
Contributor Author

Skipped the slow test on MacOS with Assume.

@basilevs basilevs force-pushed the issue_882_performanceTestOnCI branch from cc9b54b to 3f9c222 Compare November 29, 2023 21:39
This run PerformanceTests in GitHub actions for pull requests labeled
"performance".

Note, that the event of labeling itself is not handled. This
implementation implies a contributor will have to push something to PR
after labeling to start tests.
@basilevs basilevs force-pushed the issue_882_performanceTestOnCI branch from 3f9c222 to 158e4c6 Compare December 3, 2023 09:26
@basilevs
Copy link
Contributor Author

basilevs commented Dec 3, 2023

@laeubi this works fine. Please merge.

@laeubi laeubi merged commit ff9fb3c into eclipse-platform:master Dec 3, 2023
13 checks passed
@basilevs basilevs deleted the issue_882_performanceTestOnCI branch December 3, 2023 11:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
performance Performance issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants