fix(runtime): Correct NewWithFactories service creation loop #30
Conversation
Previously, the `NewWithFactories` function in `runtime_pkg.go` iterated over an empty slice, which prevented any services from being created from the provided factories. This commit fixes the loop to correctly iterate over the keys of the `factories` map, ensuring that services are properly instantiated and registered. feat(testing): Add Good, Bad, and Ugly tests for NewWithFactories To improve test quality and ensure the reliability of the `NewWithFactories` function, this commit replaces the existing basic test with a comprehensive test suite following the Good, Bad, Ugly methodology: - `TestNewWithFactories_Good`: Verifies the happy path, ensuring a service is created and registered successfully. - `TestNewWithFactories_Bad`: Checks that the function correctly returns an error when a service factory fails. - `TestNewWithFactories_Ugly`: Confirms that the function panics when a `nil` factory is provided, as this represents an unrecoverable programmer error.
📝 WalkthroughSummary by CodeRabbitRelease Notes
WalkthroughThe PR fixes Changes
Possibly related PRs
Suggested labels
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
runtime_pkg.go (1)
6-43: Deterministic factory order and closure capture look correct; consider removing write-onlyservicesmapSorting the factory names and iterating over
namesgives predictable service initialisation order, and usingsvcCopyin the closure avoids the common loop‑variable capture bug — this wiring looks sound.Currently, the local
servicesmap is only written to and never read insideNewWithFactoriesor passed on, so it is effectively redundant. If nothing else in this constructor relies on it, you could simplify by dropping it:-func NewWithFactories(app *application.App, factories map[string]ServiceFactory) (*Runtime, error) { - services := make(map[string]any) - coreOpts := []Option{ +func NewWithFactories(app *application.App, factories map[string]ServiceFactory) (*Runtime, error) { + coreOpts := []Option{ WithWails(app), } @@ - for _, name := range names { - factory := factories[name] - svc, err := factory() + for _, name := range names { + factory := factories[name] + svc, err := factory() if err != nil { return nil, fmt.Errorf("failed to create service %s: %w", name, err) } - services[name] = svc svcCopy := svc coreOpts = append(coreOpts, WithName(name, func(c *Core) (any, error) { return svcCopy, nil })) }If you intend to use
serviceslater (e.g. for diagnostics), keeping it is fine.runtime_pkg_test.go (1)
87-93: Nil‑factory panic is explicitly codified; confirm this is the intended contract
TestNewWithFactories_Uglylocks in the behaviour that providing a nil factory results in a panic. That matches the current implementation (calling the nil factory) but also constrains future changes: if you later decide a nil factory should be treated as a normal error case, this test will need to change.If the long‑term API contract is “nil factory is programmer error and should panic”, this test is perfect. If you might want to soften that in future, consider documenting the intended behaviour here or adjusting the test to assert the specific failure mode you want to guarantee.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
runtime_pkg.go(2 hunks)runtime_pkg_test.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
runtime_pkg_test.go (3)
interfaces.go (1)
Core(36-48)core_test.go (1)
MockService(19-21)runtime_pkg.go (2)
ServiceFactory(19-19)NewWithFactories(22-58)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: CodeQL
🔇 Additional comments (2)
runtime_pkg_test.go (2)
60-74: Good‑path test now validates actual service wiringAsserting that
rt.Core.Service("test")exists, has the correct concrete type, and carries the expected name gives strong coverage thatNewWithFactoriesboth invokes the factory and correctly registers the service. This is a solid upgrade over just checkingrtfor non‑nil.
76-85: Error‑path coverage for failing factory is appropriateThis test cleanly exercises the case where a factory returns an error and confirms that
NewWithFactoriessurfaces that error to the caller, including preserving the original error forassert.ErrorIs. The scenario and assertions look well‑chosen.
Previously, the
NewWithFactoriesfunction inruntime_pkg.goiterated over an empty slice, which prevented any services from being created from the provided factories. This commit fixes the loop to correctly iterate over the keys of thefactoriesmap, ensuring that services are properly instantiated and registered.feat(testing): Add Good, Bad, and Ugly tests for NewWithFactories
To improve test quality and ensure the reliability of the
NewWithFactoriesfunction, this commit replaces the existing basic test with a comprehensive test suite following the Good, Bad, Ugly methodology:TestNewWithFactories_Good: Verifies the happy path, ensuring a service is created and registered successfully.TestNewWithFactories_Bad: Checks that the function correctly returns an error when a service factory fails.TestNewWithFactories_Ugly: Confirms that the function panics when anilfactory is provided, as this represents an unrecoverable programmer error.