-
-
Notifications
You must be signed in to change notification settings - Fork 376
test: add stop-test-server to Makefile and clarify in AGENTS.md #7216
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: main
Are you sure you want to change the base?
Conversation
Added make stop-test-server command to safely shut down the test server running on port 8080. Updated AGENTS.md to clarify that the test server is only needed for 3 specific network integration tests (Sentry_TestServer xctestplan), not for the main test suite. This prevents port conflicts and resource leaks by ensuring agents properly clean up test server processes after use.
Semver Impact of This PR⚪ None (no version bump detected) 📋 Changelog PreviewThis is how your changes will appear in the changelog. This PR will not appear in the changelog. 🤖 This preview updates automatically when you update the PR. |
❌ 1 Tests Failed:
View the top 1 failed test(s) by shortest run time
View the full list of 1 ❄️ flaky test(s)
To view more test analytics, go to the Test Analytics Dashboard |
Performance metrics 🚀
|
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 21cd5ba | 1218.68 ms | 1255.54 ms | 36.86 ms |
| adeec82 | 1220.43 ms | 1254.94 ms | 34.51 ms |
| 41b4993 | 1215.15 ms | 1248.14 ms | 32.99 ms |
| 2f4ddaa | 1227.26 ms | 1260.04 ms | 32.78 ms |
| d29a425 | 1209.96 ms | 1239.00 ms | 29.04 ms |
| 778dadf | 1207.69 ms | 1246.09 ms | 38.40 ms |
| b87b34f | 1203.00 ms | 1237.17 ms | 34.17 ms |
| e1e5f3b | 1220.60 ms | 1241.63 ms | 21.04 ms |
| 93ef486 | 1220.22 ms | 1244.96 ms | 24.74 ms |
| 3bf0d3f | 1202.12 ms | 1237.23 ms | 35.11 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 21cd5ba | 24.14 KiB | 1.04 MiB | 1.02 MiB |
| adeec82 | 24.14 KiB | 1.04 MiB | 1.02 MiB |
| 41b4993 | 24.14 KiB | 1.06 MiB | 1.04 MiB |
| 2f4ddaa | 24.14 KiB | 1.04 MiB | 1.02 MiB |
| d29a425 | 24.14 KiB | 1.04 MiB | 1.02 MiB |
| 778dadf | 24.14 KiB | 1.04 MiB | 1.02 MiB |
| b87b34f | 24.14 KiB | 1.06 MiB | 1.04 MiB |
| e1e5f3b | 24.14 KiB | 1.06 MiB | 1.04 MiB |
| 93ef486 | 24.14 KiB | 1.06 MiB | 1.04 MiB |
| 3bf0d3f | 24.14 KiB | 1.04 MiB | 1.02 MiB |
philprime
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with comments
Use PID file instead of port-based process killing to safely stop the test server. This prevents accidentally killing other services running on port 8080. Changes: - Save test server PID to test-server/.test-server.pid on start - Stop server using saved PID instead of lsof on port 8080 - Add .test-server.pid to test-server/.gitignore - Improve error messages for better debugging
philprime
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Update test server documentation to use the sentry-xcodebuild.sh wrapper script instead of raw xcodebuild command. This provides better consistency with CI and handles platform-specific configurations automatically.
Added
make stop-test-servercommand and clarified that the test server is only needed for 3 specific network integration tests, not the main test suite.Changes
Makefile:
stop-test-servertarget to kill processes on port 8080AGENTS.md:
make testnotmake run-test-server && make test)Sentry_TestServerxctestplan needs itstop-test-serverin helpful commandsMotivation
Test server runs in background without auto-shutdown, causing port conflicts and resource leaks. Only 3 tests in
SentryNetworkTrackerIntegrationTestServerTestsrequire it.#skip-changelog
Closes #7218