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

test: Add coverage for runner health monitor #39

Merged
merged 14 commits into from
Dec 2, 2024

Conversation

ivov
Copy link
Contributor

@ivov ivov commented Nov 27, 2024

@codecov-commenter
Copy link

codecov-commenter commented Nov 28, 2024

Codecov Report

Attention: Patch coverage is 81.81818% with 4 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
internal/commands/launch.go 0.00% 2 Missing ⚠️
internal/http/manage_runner_health.go 90.00% 1 Missing and 1 partial ⚠️

📢 Thoughts on this report? Let us know!

@ivov ivov marked this pull request as ready for review November 28, 2024 16:59
@ivov
Copy link
Contributor Author

ivov commented Nov 28, 2024

There might be a timing issue here, I'll take a closer look tomorrow.

Copy link
Contributor

@tomi tomi left a comment

Choose a reason for hiding this comment

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

Suggested how to make this hopefully a bit more testable. LMK WDYT

@ivov ivov requested a review from tomi November 29, 2024 09:34
tomi
tomi previously approved these changes Nov 29, 2024
Copy link
Contributor

@tomi tomi left a comment

Choose a reason for hiding this comment

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

Much cleaner now 👍

panic(fmt.Errorf("failed to terminate runner process: %v", err))
}
logs.Debug("Stopped monitoring runner health")
logs.Warn("Found runner unresponsive too many times, terminating runner...")
Copy link
Contributor

Choose a reason for hiding this comment

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

Since this no longer does the termination, we could move this log to where it's actually terminated

Makefile Outdated
Comment on lines 31 to 32
test-uncached:
go test -count=1 ./...
Copy link
Contributor

Choose a reason for hiding this comment

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

What does this do? Could we add a comment?

Copy link
Contributor Author

@ivov ivov Nov 29, 2024

Choose a reason for hiding this comment

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

Added while testing but not really useful, removed.

Edit: To elaborate, Go caches successful test results by default and I was checking if there were any timing issues by running the same tests multiple times. No need now that this was all refactored.

Copy link
Contributor

@tomi tomi left a comment

Choose a reason for hiding this comment

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

🚀

@ivov ivov merged commit 4ce8065 into main Dec 2, 2024
4 checks passed
@ivov ivov deleted the add-coverage-for-runner-health-monitor branch December 2, 2024 14:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants