chore: refactor CI workflow and add test coverage for Oumi and MCP server#16
Conversation
Tests: - Add unit tests for oumi/serve.py and generate_training_data.py (72 tests) - Add JS tests for ui/lib/kestra.js - Add MCP server integration test Fixes: - Add missing `import json` in oumi/generate_training_data.py - Update license to MIT Cleanup: - Remove redundant test files and unused scripts - Remove compute_reward.py and simulate_load.py - Update CI workflow to run actual tests - Add requirements.txt
WalkthroughThis PR rebrand the project from CloudGenesis to InFoundry, restructures CI/CD workflows to build MCP server and lint UI separately, updates licensing to MIT, removes placeholder microservices, adds comprehensive test coverage, and rewrites documentation with concrete setup instructions and architecture details. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–25 minutes Areas requiring extra attention:
Possibly related PRs
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
Comment |
🐰 CodeRabbit Review ReminderThis PR will be automatically reviewed by CodeRabbit. Please ensure:
Maintainer approval is required after addressing CodeRabbit feedback. |
|
Tip For best results, initiate chat on the files or code changes. Hey there! 👋 I see this automated reminder went out. Just to let you know, I'm here and ready to help! Since I don't have context from earlier comments in this thread, I'm responding fresh to this reminder. This PR looks like it's got some solid work - tests, cleanup, and bug fixes. If you'd like me to do a full review of all the changes in this PR, just let me know and I'll dive in! You can either:
I'm here to help make sure everything looks good before merging. What would you like me to do? 🐰 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
docs/architecture.md (1)
7-25: Optional: Quiet the markdown linter.The static analysis tool is complaining that the fenced code block (the ASCII diagram) doesn't have a language specified. You can add
textorplaintextafter the opening triple backticks to make it happy, but this is totally optional since it's just documentation formatting.docs/runlocally.md (1)
62-62: Optional: Format the URL properly.Markdown linters prefer URLs to be either in angle brackets
<https://...>or as proper links[text](url). No big deal, but it's a small best practice thing.-1. Install Kestra: https://kestra.io/docs/installation +1. Install Kestra: <https://kestra.io/docs/installation>README.md (1)
53-63: Optional: Add language specifier to the code block.The markdown linter wants a language specified for the project structure tree. You can add
textafter the opening triple backticks to make it happy:-``` +```text infoundry/ ├── docs/ # DocumentationNot a big deal, just keeps the linter quiet!
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (29)
.github/workflows/ci.yml(1 hunks).gitignore(2 hunks)CODE_OF_CONDUCT.md(1 hunks)CONTRIBUTING.md(2 hunks)LICENSE(1 hunks)README.md(1 hunks)docs/architecture.md(1 hunks)docs/runlocally.md(1 hunks)examples/telemetry/metrics.json(0 hunks)infoundry-mcp-server/README.md(1 hunks)infoundry-mcp-server/package.json(1 hunks)infoundry-mcp-server/test.mjs(0 hunks)infra/.gitkeep(0 hunks)infra/modules/.gitkeep(0 hunks)infra/templates/.gitkeep(0 hunks)oumi/generate_training_data.py(1 hunks)requirements.txt(1 hunks)scripts/compute_reward.py(0 hunks)scripts/simulate_load.py(0 hunks)services/auth/Dockerfile(0 hunks)services/auth/app.py(0 hunks)services/payments/Dockerfile(0 hunks)services/payments/app.py(0 hunks)tests/.gitkeep(0 hunks)tests/test_generate_training_data.py(1 hunks)tests/test_kestra.js(1 hunks)tests/test_mcp_server.mjs(2 hunks)tests/test_oumi_serve.py(1 hunks)ui/README.md(1 hunks)
💤 Files with no reviewable changes (12)
- scripts/compute_reward.py
- tests/.gitkeep
- services/auth/Dockerfile
- scripts/simulate_load.py
- infra/.gitkeep
- infra/modules/.gitkeep
- services/payments/app.py
- services/payments/Dockerfile
- services/auth/app.py
- infra/templates/.gitkeep
- infoundry-mcp-server/test.mjs
- examples/telemetry/metrics.json
🧰 Additional context used
📓 Path-based instructions (1)
**/*.md
⚙️ CodeRabbit configuration file
Do not emphasize these files too much
Files:
ui/README.mdCODE_OF_CONDUCT.mddocs/runlocally.mdCONTRIBUTING.mdinfoundry-mcp-server/README.mdREADME.mddocs/architecture.md
🧬 Code graph analysis (2)
tests/test_kestra.js (2)
ui/components/StepOutputCard.jsx (1)
step(318-318)ui/lib/kestra.js (1)
calculateDuration(132-143)
tests/test_generate_training_data.py (1)
oumi/generate_training_data.py (5)
determine_pattern(81-139)generate_components(142-183)generate_rationale(186-235)generate_example(238-364)main(429-494)
🪛 markdownlint-cli2 (0.18.1)
docs/runlocally.md
62-62: Bare URL used
(MD034, no-bare-urls)
README.md
53-53: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
docs/architecture.md
53-53: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🪛 Ruff (0.14.8)
tests/test_generate_training_data.py
152-152: Local variable result is assigned to but never used
Remove assignment to unused variable result
(F841)
156-156: Loop control variable comp not used within loop body
Rename unused comp to _comp
(B007)
🔇 Additional comments (15)
.gitignore (1)
4-6: Nice organization on the .gitignore update!The new entries follow solid conventions for Python + Node projects. Love that you grouped them with section comments (Logs, IDE, Test coverage, Compiled files)—that makes it really easy to scan and maintain. All the patterns make sense: virtual env dirs, log files, IDE artifacts (.idea/, .vscode/, vim swaps), pytest coverage outputs, and Python package metadata. This should keep your repo clean without accidentally ignoring anything important.
Also applies to: 16-32
LICENSE (1)
1-21: License change looks good!The MIT license text is complete and correct, and it's consistent with the license field update in package.json. Clean branding update.
infoundry-mcp-server/package.json (1)
16-16: License field update is consistent!Matches the root LICENSE file change. Simple and aligned with the rebranding.
CODE_OF_CONDUCT.md (1)
20-20: Smart contact change!Replacing a hard-coded email with "the project maintainers" reduces maintenance friction if contact methods need to change later. Good practice for long-term project health.
requirements.txt (1)
1-13: Dependency selection is solid!FastAPI + Uvicorn + Pydantic is the standard modern Python web stack, and pytest + httpx are great for testing. The commented-out oumi[gpu] is a nice touch for optional local development.
One quick note: Since these version constraints predate late 2024, you might want to double-check that these are still the latest stable versions. The pins aren't super strict (just
>=), so you've got room to upgrade. Want me to help verify the current versions?CONTRIBUTING.md (1)
28-59: CodeRabbit section is well-structured!The expanded guidance on IaC validation, merge blocking, and interaction patterns is clear and helpful. This gives contributors a much better sense of how the review process works.
ui/README.md (1)
1-49: Documentation upgrade is really clean!The restructuring from generic getting-started to a feature-focused layout with clear dev setup, Pages table, and environment variables is way more helpful. The .env.local example for KESTRA_API_URL is especially nice — saves contributors a config headache. Good work on the Next.js 15 bump too.
.github/workflows/ci.yml (3)
9-26: Python test job looks great!Separate Python test job with proper requirements.txt installation, pytest with verbose output, and the right ignore flags for JS test files. Clean setup.
28-43: MCP server build job is well-isolated!Node 20 setup, npm ci (clean installs are best practice in CI), and npm run build in the right directory. Solid separation of concerns.
45-62: Clarify intent of|| truein the lint step.Line 62 has
npm run lint || true, which means linting warnings won't fail the job. This is sometimes intentional (to allow warnings without blocking merges) but it can also hide real issues.A few questions to clarify:
- Are lint warnings intentionally non-blocking? If so, awesome — just make sure your team knows warnings exist (maybe log them).
- Should this be stricter? If lint errors should block merges, remove the
|| true.The
|| truepattern is a common gotcha in CI configs, so just making sure it's intentional!infoundry-mcp-server/README.md (1)
64-65: Documentation looks solid!Nice update to reflect running tests from the project root. This aligns perfectly with the refactored test script structure.
tests/test_mcp_server.mjs (1)
15-18: Nice refactor to support project-root test execution!Adding the
mcpServerDirconstant and using it as the working directory for the spawned process is clean. This lets you run the test from anywhere in the project while ensuring the MCP server starts up in the right context.tests/test_oumi_serve.py (1)
1-292: Excellent test coverage for the Oumi serve API!This is a really solid test suite! You've got 29 tests covering:
- The heuristic response logic with different pattern triggers (Kafka, Kubernetes, serverless, etc.)
- Health endpoint validation
- Chat completions with both happy and sad paths (nice 400 test!)
- Response structure validation including JSON parsing
The test organization into classes is clean, and your docstrings make it super clear what each test is checking. This kind of comprehensive testing is exactly what you want to see in production code.
tests/test_kestra.js (1)
1-260: Great test coverage for the Kestra utilities!You've done a nice job testing all the key utility functions here. I especially like:
- The comprehensive STATE_MAP tests covering all the lifecycle states
- The edge case handling in
calculateDuration(null dates, sub-second durations, etc.)- The
mapToStepProgresstests validating both the happy path and missing data scenariosUsing the native Node.js test runner (
node:test) is a solid choice too - keeps dependencies minimal. The test structure is easy to follow with clear describe blocks and descriptive test names.tests/test_generate_training_data.py (1)
1-149: Solid test suite overall!Outside of that one incomplete test, this is really comprehensive coverage of the training data generation functions. You've got:
- All the pattern determination logic tested (GPU, Kafka, service counts, etc.)
- Component generation with database and queue mappings
- Rationale generation for all patterns
- Example generation with JSON validation
The test organization is clean, and I like how you're testing the randomness in
generate_example()by running it multiple times. Good thinking!Also applies to: 159-387
crypticsaiyan
changed the title
1 participant
Description
Add comprehensive unit tests for core services, cleanup unused files, and fix bugs to improve code quality and maintainability.
Type of Change
Changes Made
Testing
terraform validate) passesTest Evidence
============================== 72 passed in 5.33s ==============================Checklist
CodeRabbit Review
This PR will be automatically reviewed by CodeRabbit 🐰
Summary by CodeRabbit
Documentation
Chores
✏️ Tip: You can customize this high-level summary in your review settings.