obs-status-service: add --test-run using interface-based Redis mock (fixes #113) #123

schrodinger · 2026-01-27T15:01:44+01:00

schrodinger commented

2026-01-27 15:01:44 +01:00

First-time contributor

This PR adds a --test-run mode to obs-status-service to allow local development without requiring access to OBS Redis.

The implementation follows an interface-based design for the Redis client, allowing a clean separation between the real Redis client and a mock implementation (redismock.go), while keeping production logic unchanged.

This approach avoids conditional logic in the core code paths and aligns with the design discussed in #113.

Fixes #113.

This PR adds a --test-run mode to obs-status-service to allow local development without requiring access to OBS Redis. The implementation follows an interface-based design for the Redis client, allowing a clean separation between the real Redis client and a mock implementation (redismock.go), while keeping production logic unchanged. This approach avoids conditional logic in the core code paths and aligns with the design discussed in #113. Fixes #113.

schrodinger added 1 commit 2026-01-27 15:01:44 +01:00

obs-status-service: add --test-run using interface-based Redis mock (fixes #113 )

go-generate-check / go-generate-check (pull_request) Has been cancelled

Details

bb37ea8718

adamm reviewed 2026-01-27 15:41:56 +01:00

obs-status-service/redis.go Outdated

						
				@@ -21,2 +26,3 @@

					if err != nil {

						panic(err)

						common.LogError("Invalid Redis URL", err)

						return

adamm commented

2026-01-27 15:41:56 +01:00

Why no more panic()?

schrodinger commented

2026-01-27 17:53:59 +01:00

First-time contributor

Why no more panic()?

I removed panic because I think that this code runs in a long-lived service so like any invalid Redis URL might give a runtime error, so logging the error and exiting cleanly felt more appropriate than crashing with a stack trace.

This also keeps the behavior consistent with the --test-run mode, where we want predictable startup failures instead of panics.

If you prefer keeping panic here for consistency with the rest of the codebase, I’m happy to switch it back.

> Why no more panic()? I removed panic because I think that this code runs in a long-lived service so like any invalid Redis URL might give a runtime error, so logging the error and exiting cleanly felt more appropriate than crashing with a stack trace. This also keeps the behavior consistent with the --test-run mode, where we want predictable startup failures instead of panics. If you prefer keeping panic here for consistency with the rest of the codebase, I’m happy to switch it back.

adamm commented

2026-02-02 11:54:39 +01:00

Invalid Redis URL means we cannot start so it's fine to panic(). in *testRun you're already not calling this function anyway. Maybe have an error log here too and then panic.

It's long running only when it has an actual URL to try.

Invalid Redis URL means we cannot start so it's fine to panic(). in *testRun you're already not calling this function anyway. Maybe have an error log here too and then panic. It's long running only when it has an actual URL to try.

schrodinger commented

2026-02-02 15:21:23 +01:00

First-time contributor

Ok I understand, thanks for the clarification!
I’ve updated RedisConnect to panic on an invalid Redis URL.

Ok I understand, thanks for the clarification! I’ve updated RedisConnect to panic on an invalid Redis URL.

schrodinger requested review from adamm 2026-01-27 17:54:10 +01:00

schrodinger force-pushed issue-113 from bb37ea8718 to ac19fecf01

2026-01-29 16:31:18 +01:00

Compare

schrodinger force-pushed issue-113 from ac19fecf01 to d563763f63

2026-02-02 15:19:58 +01:00

Compare

schrodinger force-pushed issue-113 from d563763f63 to 7730b39b0f

2026-02-06 05:16:12 +01:00

Compare

dgarcia requested changes 2026-02-11 14:02:47 +01:00

dgarcia left a comment

First-time contributor

One improvement that we can do here is to use the current existing testing files instead of hardcoding the values.

We've two json files (factory.results.json.bz2 and gcc15.results.json.bz2) already in the repository with sample data like:

[
 {
 "Project": "openSUSE:Factory",
 "Repository": "standard",
 "Arch": "x86_64",
 "Status": [
   {
    "Package": "python313",
    "Code": "succeeded",
    ...
   },
   ...
 ]
 },
 ...
]

So what can we do is to unzip those files, maybe just factory.results.json.bz2, like what we are doing right now in main_test.go, and iterate over the BuildResult array to build the mock return.

One improvement that we can do here is to use the current existing testing files instead of hardcoding the values. We've two json files (`factory.results.json.bz2` and `gcc15.results.json.bz2`) already in the repository with sample data like: ```json [ { "Project": "openSUSE:Factory", "Repository": "standard", "Arch": "x86_64", "Status": [ { "Package": "python313", "Code": "succeeded", ... }, ... ] }, ... ] ``` So what can we do is to unzip those files, maybe just `factory.results.json.bz2`, like what we are doing right now in `main_test.go`, and iterate over the `BuildResult` array to build the mock return.

schrodinger force-pushed issue-113 from 7730b39b0f to 3e69597d66

2026-02-24 20:55:38 +01:00

Compare

schrodinger commented

2026-02-24 21:02:20 +01:00

First-time contributor

I understand the suggestion.
I’ve refactored the mock to reuse the existing .json.bz2 test data instead of hardcoded values. The mock now loads factory.results.json.bz2, parses the BuildResult array, and dynamically generates the ScanType and HGetAll responses from it.

I understand the suggestion. I’ve refactored the mock to reuse the existing .json.bz2 test data instead of hardcoded values. The mock now loads factory.results.json.bz2, parses the BuildResult array, and dynamically generates the ScanType and HGetAll responses from it.

schrodinger requested review from dgarcia 2026-02-24 21:02:33 +01:00

dgarcia reviewed 2026-02-27 12:37:24 +01:00

dgarcia left a comment

First-time contributor

Looks good. It's true that instead of iterating over the results and building the return data for every request, we can do that once and store the pre-calculated data. But this is for testing so we don't need to be performant.

schrodinger force-pushed issue-113 from 3e69597d66 to feb589f571

2026-02-27 17:25:33 +01:00

Compare

schrodinger commented

2026-02-27 18:35:46 +01:00

First-time contributor

I rebased this branch onto the latest main, after that the integration tests started failing in some workflow-related tests.

This PR only touches obs-status-service, so I’m not sure if the failures are related to these changes. I’m seeing the same failures on my issue-114 branch as well after rebasing.

Just wanted to flag it, let me know if you’d like me to dig into it further.

I rebased this branch onto the latest main, after that the integration tests started failing in some workflow-related tests. This PR only touches obs-status-service, so I’m not sure if the failures are related to these changes. I’m seeing the same failures on my issue-114 branch as well after rebasing. Just wanted to flag it, let me know if you’d like me to dig into it further.

go-generate-check / go-generate-check (pull_request) Successful in 7s

Details

Integration tests / t (pull_request) Failing after 4m51s

Details

This pull request can be merged automatically.

You are not authorized to merge this pull request.

View command line instructions

Checkout

From your project repository, check out a new branch and test the changes.

git fetch -u issue-113:schrodinger-issue-113

git checkout schrodinger-issue-113

Sign in to join this conversation.

3 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: git-workflow/autogits#123

Allow edits from maintainers