Skip to content

Refactor Image(Display, int, int) in Tests (No Ops) #2221

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jul 3, 2025
Merged

Refactor Image(Display, int, int) in Tests (No Ops) #2221

merged 1 commit into from
Jul 3, 2025

Conversation

ShahzaibIbrahim
Copy link
Contributor

@ShahzaibIbrahim ShahzaibIbrahim commented Jun 10, 2025 •
edited
Loading

Replacing Image(Display, int, int) with Image(Display, ImageGcDrawer, int, int) in Tests. Basic Case

@ShahzaibIbrahim ShahzaibIbrahim mentioned this pull request Jun 10, 2025
Closed
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Jun 10, 2025 •
edited
Loading

Test Results

   545 files  ±0     545 suites  ±0   32m 32s ⏱️ + 2m 44s
 4 405 tests ±0   4 387 ✅ ±0   18 💤 ±0  0 ❌ ±0 
16 741 runs  ±0  16 599 ✅ ±0  142 💤 ±0  0 ❌ ±0 

Results for commit 06fbe94. ± Comparison against base commit 3d93396.

♻️ This comment has been updated with latest results.

@ShahzaibIbrahim ShahzaibIbrahim linked an issue Jun 11, 2025 that may be closed by this pull request
Replace usages of new Image(device, width, height) for Snippets/Tests vi-eclipse/Eclipse-Platform#271
Closed
4 tasks
HeikoKlare
HeikoKlare previously requested changes Jun 16, 2025
View reviewed changes
Copy link
Contributor

@HeikoKlare HeikoKlare left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR overlaps with #2229. Please consider the comments I made there and please revise the PRs so that they do not change the same lines of code in the semantically same but syntactically different ways.

@ShahzaibIbrahim ShahzaibIbrahim force-pushed the master-271-noOp branch 2 times, most recently from 242be4b to f724256 Compare June 20, 2025 14:25
@ShahzaibIbrahim ShahzaibIbrahim marked this pull request as draft June 20, 2025 14:26
@ShahzaibIbrahim ShahzaibIbrahim marked this pull request as ready for review June 20, 2025 14:38
@fedejeanne fedejeanne mentioned this pull request Jun 23, 2025
Closed
4 tasks
@fedejeanne
Copy link
Contributor

@ShahzaibIbrahim is there anything else to do here from your side or have all comments been addressed? I see that some conversations are still open.

Also, the test org.eclipse.swt.tests.junit.Test_org_eclipse_swt_SWT.test_isLocal failed failed in 2 consecutive runs. Can you look into that? If it's a flaky test, please report it.

@ShahzaibIbrahim
Copy link
Contributor Author

@ShahzaibIbrahim is there anything else to do here from your side or have all comments been addressed? I see that some conversations are still open.

Also, the test org.eclipse.swt.tests.junit.Test_org_eclipse_swt_SWT.test_isLocal failed failed in 2 consecutive runs. Can you look into that? If it's a flaky test, please report it.

All the comments were addressed but it needs to be re-reviewed.

@fedejeanne fedejeanne dismissed HeikoKlare’s stale review July 2, 2025 10:52

The comments in #2229 have been addressed

@ShahzaibIbrahim
Copy link
Contributor Author

@akurtakov the test test_isLocal fails for this PR only in the CI. Do you have any idea what is it about?

@fedejeanne
Copy link
Contributor

@akurtakov the test test_isLocal fails for this PR only in the CI. Do you have any idea what is it about?

My educated guess is that this check fails because SWT was not built for Linux in this PR and the check basically says "you're not testing your changes, you're testing master right now".

But this is irrelevant since this PR doesn't change any production code that needs to be tested, it changes only the tests. So technically, it is OK to test master (more precisely: the latest official drop).

Am I seeing this right, @akurtakov ?

@akurtakov
Copy link
Member

I am traveling so I can't look into things this week. Please remind me next week if there is still a problem to look into it.

@ShahzaibIbrahim @akurtakov
Refactor Image(Display, int, int) in Tests (No Ops)
06fbe94 
Replacing Image(Display, int, int) with Image(Display, ImageGcDrawer,
int, int) in Tests. Basic Case.
@fedejeanne
Copy link
Contributor

Thank you for fixing it, @akurtakov !

May I ask what was the trick? If you merely rebased on master then you must have magic hands because I tried that too and it didn't work... 😅

@fedejeanne fedejeanne merged commit 7100fa5 into eclipse-platform:master Jul 3, 2025
17 checks passed
@akurtakov
Copy link
Member

I have just rebased it :) . My gut feeling is that this error appears in a period when a commit makes the build produce different artifact compared to last I-build or smth similar and it starts working after the next build is in I-build repo. I haven't looked whether that's the case so I could be totally wrong too.

@ShahzaibIbrahim ShahzaibIbrahim deleted the master-271-noOp branch July 3, 2025 14:56
@fedejeanne
Copy link
Contributor

Ok, but at least I have something to try the next time I see this check fail. Thank you for the hint!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fedejeanne fedejeanne fedejeanne left review comments

@HeikoKlare HeikoKlare HeikoKlare left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Replace usages of new Image(device, width, height) for Snippets/Tests
4 participants
@ShahzaibIbrahim @fedejeanne @akurtakov @HeikoKlare