test: verify that configuration is loaded correctly

aahlenst commented

2026-01-21 16:35:32 +00:00

Member

Adds tests that verify that the runner configuration is loaded correctly, with and without a configuration file.

other
- PR: test: verify that configuration is loaded correctly

Adds tests that verify that the runner configuration is loaded correctly, with and without a configuration file.   - other - [PR](https://code.forgejo.org/forgejo/runner/pulls/1312): test: verify that configuration is loaded correctly

aahlenst added 2 commits

2026-01-21 16:35:32 +00:00

test: verify that configuration is loaded correctly 40da0132b5

test: remove redundant tests

checks / validate pre-commit-hooks file (pull_request) Successful in 37s

Details

checks / validate mocks (pull_request) Successful in 44s

Details

checks / Build Forgejo Runner (pull_request) Successful in 44s

Details

checks / runner exec tests (pull_request) Successful in 32s

Details

checks / Build unsupported platforms (pull_request) Successful in 59s

Details

checks / integration tests (docker-latest) (pull_request) Successful in 8m52s

Details

checks / integration tests (docker-stable) (pull_request) Successful in 11m19s

Details

cascade / forgejo (pull_request_target) Has been skipped

Details

cascade / debug (pull_request_target) Has been skipped

Details

cascade / end-to-end (pull_request_target) Has been skipped

Details

issue-labels / release-notes (pull_request_target) Successful in 10s

Details

a827a06bdb

aahlenst commented

2026-01-21 16:36:49 +00:00

Author

Member

The tests do not cover compatibleWithOldEnvs() which has been deprecated for almost three years. NetworkMode is only tested lightly for the same reason. I'd prefer removing them instead of adding tests. When would be a good time for that?

I thought about converting LoadDefault() to functional options so that we can write Load() (would produce the default configuration) or Load(FromFile("path/to/runner/configuration")). It would be more flexible and easier to test. However, it doesn't mix well with the lower part of LoadDefault() that silently converts invalid values into default values which in itself is a problem.

It's likely that there's more room for improvement. I'm happy to improve the state of affairs but I need some inspiration.

The tests do not cover `compatibleWithOldEnvs()` which has been deprecated for almost three years. `NetworkMode` is only tested lightly for the same reason. I'd prefer removing them instead of adding tests. When would be a good time for that? I thought about converting `LoadDefault()` to functional options so that we can write `Load()` (would produce the default configuration) or `Load(FromFile("path/to/runner/configuration"))`. It would be more flexible and easier to test. However, it doesn't mix well with the lower part of `LoadDefault()` that silently converts invalid values into default values which in itself is a problem. It's likely that there's more room for improvement. I'm happy to improve the state of affairs but I need some inspiration.

mfenniak merged commit 6a9a16de04 into main

2026-01-22 16:17:08 +00:00

mfenniak referenced this pull request from a commit

2026-01-22 16:17:09 +00:00

test: verify that configuration is loaded correctly (#1312)

mfenniak commented

2026-01-22 16:22:41 +00:00

Owner

@aahlenst wrote in #1312 (comment):

The tests do not cover compatibleWithOldEnvs() which has been deprecated for almost three years. NetworkMode is only tested lightly for the same reason. I'd prefer removing them instead of adding tests. When would be a good time for that?

My first thought is to create a PR that removes them and we can bundle it with the next time we're forced to do a major release for a breaking change, and we can time-limit it and release it in a few months if nothing else comes up to force it. But if you're going to be active in this area for other changes, the open PR may be conflict prone.

I have the overall feeling that we're annoying users with major releases, even though it's been a couple months since the last. But until this recent 12.x period, they were somewhat aggressive, and I think I'm still trying to avoid them to prevent inflaming that annoyance.

@aahlenst wrote in https://code.forgejo.org/forgejo/runner/pulls/1312#issuecomment-74675: > The tests do not cover `compatibleWithOldEnvs()` which has been deprecated for almost three years. `NetworkMode` is only tested lightly for the same reason. I'd prefer removing them instead of adding tests. When would be a good time for that? My first thought is to create a PR that removes them and we can bundle it with the next time we're forced to do a major release for a breaking change, and we can time-limit it and release it in a few months if nothing else comes up to force it. But if you're going to be active in this area for other changes, the open PR may be conflict prone. I have the overall feeling that we're annoying users with major releases, even though it's been a couple months since the last. But until this recent 12.x period, they were somewhat aggressive, and I think I'm still trying to avoid them to prevent inflaming that annoyance.

mfenniak added the

Kind/Testing

label

2026-01-22 16:32:57 +00:00

aahlenst commented

2026-01-22 21:14:30 +00:00

Author

Member

@mfenniak wrote in #1312 (comment):

My first thought is to create a PR that removes them and we can bundle it with the next time we're forced to do a major release for a breaking change, and we can time-limit it and release it in a few months if nothing else comes up to force it. But if you're going to be active in this area for other changes, the open PR may be conflict prone.

For the moment, it might be sufficient to collect those in an issue and remove the deprecated code shortly before a major release. We only have to remember to do it.

I have the overall feeling that we're annoying users with major releases, even though it's been a couple months since the last. But until this recent 12.x period, they were somewhat aggressive, and I think I'm still trying to avoid them to prevent inflaming that annoyance.

Less churn is good. Even one minor release per month is a bit much. It also gives us less time to play around with new features. On the other hand, we seem to need more releases to get newer versions of jobparser into Forgejo.

@mfenniak wrote in https://code.forgejo.org/forgejo/runner/pulls/1312#issuecomment-74942: > My first thought is to create a PR that removes them and we can bundle it with the next time we're forced to do a major release for a breaking change, and we can time-limit it and release it in a few months if nothing else comes up to force it. But if you're going to be active in this area for other changes, the open PR may be conflict prone. For the moment, it might be sufficient to collect those in an issue and remove the deprecated code shortly before a major release. We only have to remember to do it. > I have the overall feeling that we're annoying users with major releases, even though it's been a couple months since the last. But until this recent 12.x period, they were somewhat aggressive, and I think I'm still trying to avoid them to prevent inflaming that annoyance. Less churn is good. Even one minor release per month is a bit much. It also gives us less time to play around with new features. On the other hand, we seem to need more releases to get newer versions of jobparser into Forgejo.

👍 1

mfenniak referenced this pull request

2026-01-22 21:39:07 +00:00

Deprecation Cleanup for Major Release v13 #1319