mirror of
https://github.com/azaion/ai-training.git
synced 2026-04-22 11:36:36 +00:00
Refine coding standards and testing guidelines
- Updated the coding rule descriptions to emphasize readability, meaningful comments, and test verification. - Revised guidelines to clarify the importance of avoiding boilerplate while maintaining readability. - Enhanced the testing rules to set a minimum coverage threshold of 75% for business logic and specified criteria for test scenarios. - Introduced a mechanism for handling skipped tests, categorizing them as legitimate or illegitimate, and outlined resolution steps. These changes aim to improve code quality, maintainability, and testing effectiveness.
This commit is contained in:
@@ -102,32 +102,46 @@ After investigating, present:
|
||||
- If user picks A → apply fixes, then re-run (loop back to step 2)
|
||||
- If user picks B → return failure to the autopilot
|
||||
|
||||
**Any test skipped** → this is also a **blocking gate**. Skipped tests mean something is wrong — either with the test, the environment, or the test design. **Never blindly remove a skipped test.** Always investigate the root cause first.
|
||||
**Any skipped test** → classify as legitimate or illegitimate before deciding whether to block.
|
||||
|
||||
#### Investigation Protocol for Skipped Tests
|
||||
#### Legitimate skips (accept and proceed)
|
||||
|
||||
For each skipped test:
|
||||
The code path genuinely cannot execute on this runner. Acceptable reasons:
|
||||
|
||||
1. **Read the test code** — understand what the test is supposed to verify and why it skips.
|
||||
2. **Determine the root cause** — why did the skip condition fire?
|
||||
- Is the test environment misconfigured? (e.g., wrong ports, missing env vars, service not started correctly)
|
||||
- Is the test ordering wrong? (e.g., a fixture in an earlier test mutates shared state)
|
||||
- Is a dependency missing? (e.g., package not installed, fixture file absent)
|
||||
- Is the skip condition outdated? (e.g., code was refactored but the skip guard still checks the old behavior)
|
||||
- Is the test fundamentally untestable in the current setup? (e.g., requires Docker restart, different OS, special hardware)
|
||||
3. **Try to fix the root cause first** — the goal is to make the test run, not to delete it:
|
||||
- Fix the environment or configuration
|
||||
- Reorder tests or isolate shared state
|
||||
- Install the missing dependency
|
||||
- Update the skip condition to match current behavior
|
||||
4. **Only remove as last resort** — if the test truly cannot run in any realistic test environment (e.g., requires hardware not available, duplicates another test with identical assertions), then removal is justified. Document the reasoning.
|
||||
- Hardware not physically present (GPU, Apple Neural Engine, sensor, serial device)
|
||||
- Operating system mismatch (Darwin-only test on Linux CI, Windows-only test on macOS)
|
||||
- Feature-flag-gated test whose feature is intentionally disabled in this environment
|
||||
- External service the project deliberately does not control (e.g., a third-party API with no sandbox, and the project has a documented contract test instead)
|
||||
|
||||
#### Categorization
|
||||
For legitimate skips: verify the skip condition is accurate (the test would run if the hardware/OS were present), verify it has a clear reason string, and proceed.
|
||||
|
||||
- **explicit skip (dead code)**: Has `@pytest.mark.skip` — investigate whether the reason in the decorator is still valid. Often these are temporary skips that became permanent by accident.
|
||||
- **runtime skip (unreachable)**: `pytest.skip()` fires inside the test body — investigate why the condition always triggers. Often fixable by adjusting test order, environment, or the condition itself.
|
||||
- **environment mismatch**: Test assumes a different environment — investigate whether the test environment setup can be fixed.
|
||||
- **missing fixture/data**: Data or service not available — investigate whether it can be provided.
|
||||
#### Illegitimate skips (BLOCKING — must resolve)
|
||||
|
||||
The skip is a workaround for something we can and should fix. NOT acceptable reasons:
|
||||
|
||||
- Required service not running (database, message broker, downstream API we control) → fix: bring the service up, add a docker-compose dependency, or add a mock
|
||||
- Missing test fixture, seed data, or sample file → fix: provide the data, generate it, or ASK the user for it
|
||||
- Missing environment variable or credential → fix: add to `.env.example`, document, ASK user for the value
|
||||
- Flaky-test quarantine with no tracking ticket → fix: create the ticket (or replay via leftovers if tracker is down)
|
||||
- Inherited skip from a prior refactor that was never cleaned up → fix: clean it up now
|
||||
- Test ordering mutates shared state → fix: isolate the state
|
||||
|
||||
**Rule of thumb**: if the reason for skipping is "we didn't set something up," that's not a valid skip — set it up. If the reason is "this hardware/OS isn't here," that's valid.
|
||||
|
||||
#### Resolution steps for illegitimate skips
|
||||
|
||||
1. Classify the skip (read the skip reason and test body)
|
||||
2. If the fix is **mechanical** — start a container, install a dep, add a mock, reorder fixtures — attempt it automatically and re-run
|
||||
3. If the fix requires **user input** — credentials, sample data, a business decision — BLOCK and ASK
|
||||
4. Never silently mark the skip as "accepted" — every illegitimate skip must either be fixed or escalated
|
||||
5. Removal is a last resort and requires explicit user approval with documented reasoning
|
||||
|
||||
#### Categorization cheatsheet
|
||||
|
||||
- **explicit skip (e.g. `@pytest.mark.skip`)**: check whether the reason in the decorator is still valid
|
||||
- **conditional skip (e.g. `@pytest.mark.skipif`)**: check whether the condition is accurate and whether we can change the environment to make it false
|
||||
- **runtime skip (e.g. `pytest.skip()` in body)**: check why the condition fires — often an ordering or environment bug
|
||||
- **missing fixture/data**: treated as illegitimate unless user confirms the data is unavailable
|
||||
|
||||
After investigating, present findings:
|
||||
|
||||
|
||||
Reference in New Issue
Block a user