Skip to content

feat(tests): intercept created executions through queue proxy & kill …#12804

Open
brian-mulier-p wants to merge 5 commits intodevelopfrom
fix/flaky-fix-attempt
Open

feat(tests): intercept created executions through queue proxy & kill …#12804
brian-mulier-p wants to merge 5 commits intodevelopfrom
fix/flaky-fix-attempt

Conversation

@brian-mulier-p
Copy link
Member

…them if running after test

@brian-mulier-p brian-mulier-p requested review from a team and loicmathieu November 7, 2025 14:00
@github-project-automation github-project-automation bot moved this to To review in Pull Requests Nov 7, 2025
@brian-mulier-p brian-mulier-p force-pushed the fix/flaky-fix-attempt branch 2 times, most recently from c599806 to a74f4be Compare November 7, 2025 14:10
Copy link
Member

@loicmathieu loicmathieu left a comment

Choose a reason for hiding this comment

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

Overall I would prefer we understand why things are not ended properly but as it seems overly complicated why not going into that direction...

@github-actions
Copy link
Contributor

github-actions bot commented Nov 7, 2025

🐋 Docker image

ghcr.io/kestra-io/kestra-pr:12804
docker run --pull=always --rm -it -p 8080:8080 --user=root -v /var/run/docker.sock:/var/run/docker.sock -v /tmp:/tmp ghcr.io/kestra-io/kestra-pr:12804 server local

🧪 Java Unit Tests

TestsPassed ❌️SkippedFailedTime ⏱
Java Tests Report0 ran0 ✅0 ⚠️0 ❌

@brian-mulier-p brian-mulier-p force-pushed the fix/flaky-fix-attempt branch 3 times, most recently from 4b54afa to 16af786 Compare November 7, 2025 15:06
@brian-mulier-p
Copy link
Member Author

I replied and fix all your comments + I spotted one last issue in context handling that is now fixed.

@github-actions
Copy link
Contributor

github-actions bot commented Nov 7, 2025

Tests report quick summary:

No test report were found

Copy link
Member

@loicmathieu loicmathieu left a comment

Choose a reason for hiding this comment

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

Still not convinced by what you said about the thread local, if you're not in the test thread, you would not have access to it anyway.

I approved it, but it seems it causes more harm than good as a lot of tests are now failing

@loicmathieu
Copy link
Member

This kind of PR should only be merged if it proved to brings benefits: for ex when launching multiple time you see none of very few flaky tests

Otherwise the rebuildContext was creating a new applicationContext that doesn't contain the testExecutions
@brian-mulier-p
Copy link
Member Author

It just needs some love & polish, trust the process 👍

@brian-mulier-p
Copy link
Member Author

brian-mulier-p commented Nov 7, 2025

In the end @loicmathieu I removed the need for ThreadLocal so it's easier to understand + I added a wait after sending the killing message to be 100% sure it doesn't overflow across tests 🤞

@brian-mulier-p brian-mulier-p force-pushed the fix/flaky-fix-attempt branch 5 times, most recently from a6e424e to 00da357 Compare November 7, 2025 17:52
@brian-mulier-p brian-mulier-p force-pushed the fix/flaky-fix-attempt branch 2 times, most recently from 849a872 to 7dddc3f Compare November 7, 2025 19:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: To review

Development

Successfully merging this pull request may close these issues.

2 participants