Skip to content

feat: Enhance training flow with wrong-answer test coverage and updat… - #148

Open
umarraza086 wants to merge 1 commit into
stagingfrom
inhance-answer-logic
Open

feat: Enhance training flow with wrong-answer test coverage and updat…#148
umarraza086 wants to merge 1 commit into
stagingfrom
inhance-answer-logic

Conversation

@umarraza086

Copy link
Copy Markdown
Collaborator

…e learning logs

Pull Request: [Feature/Fix Description]

📝 Description

Closes: [JIRA-XXXX or GitHub Issue #XXX]

🎯 Type of Change

  • New feature
  • Bug fix
  • Documentation update
  • Performance improvement
  • Refactoring
  • Dependency update

🔍 Changes Made

  • Change 1
  • Change 2
  • Change 3

🧪 E2E Testing Checklist

Required before merging to staging:

  • Feature tested locally: npm run dev
  • No console errors (DevTools → Console)
  • Signup test: created test user testcoach+staging+[random]@example.com
  • Baseline assessment: answered questions, received persona (A/B/C/D)
  • Module viewing: clicked Module 1, video played
  • Content gate: video 90% watched before quiz unlocks
  • Quiz: answered questions, score calculated correctly
  • Module completion: passed at 80%+ score
  • Dashboard: shows completed modules and next module
  • Endline: accessible only after all modules passed
  • Certificate: generated and PDF downloads correctly
  • Supabase: new user appears in auth.users table
  • Supabase: new profiles/assessments created in correct tables
  • Mobile responsive: tested at 375px viewport
  • No sensitive data in code: no API keys, passwords, emails hardcoded
  • Code follows project conventions and standards

📸 Screenshots (if UI change)

🔄 Deployment Checklist

  • Tested on staging environment (if auto-deployed)
  • No breaking changes to database schema (or migrations included)
  • Environment variables documented (if new ones added)
  • Performance: page load time acceptable

🚨 Breaking Changes

  • This PR introduces breaking changes (describe below)
  • Requires data migration
  • Requires configuration change

If checked, describe the impact:

📋 Checklist

  • Code follows project standards (DEVELOPMENT_STANDARDS.md)
  • No hardcoded secrets or sensitive data
  • Tests added/updated (if applicable)
  • Documentation updated (if needed)
  • Commit messages are clear and descriptive
  • No merge conflicts with staging branch

🔗 Related Issues

  • Related to: [Issue/PR]
  • Depends on: [Issue/PR]
  • Blocked by: [Issue/PR]

👀 Reviewers

⚠️ Notes for Reviewers


DO NOT MERGE until:

  1. ✅ Code review approved
  2. ✅ All E2E tests pass (checkbox above)
  3. ✅ CI/CD (GitHub Actions) passes

Remember: This goes to staging first, then production after staging QA passes.

@SM649 SM649 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review — feat: Enhance training flow with wrong-answer test coverage

Summary: This PR improves the coaching-agent E2E test harness with three focused changes: (1) fix a response-body truncation bug that broke answer-key parsing for large training payloads, (2) add a deliberate wrong-answer injection for coverage of the error/feedback path, and (3) unlock the full baseline flow on configured accounts when baseline is incomplete.


What's good

  • net-monitor truncation fix is the right call. Bumping the cap to 5 MB for content-api responses (and leaving 6 KB for everything else) is a clean, targeted fix. The root-cause analysis in LEARNING.md is well-documented.
  • correctAnswersForUnit() refactor — making it resilient to three payload shapes (per-unit object, per-unit array, bulk list) is a solid improvement. The tryParse helper and the ordered fallback (per-unit → bulk) are clear and safe.
  • wrongInjected run-wide flag correctly guarantees at most one wrong answer per run. The NO_WRONG=1 and WRONG_PROB env knobs are good escape hatches.
  • LEARNING.md is well-maintained and the open-items section is honest about what's still broken.

Issues to address

1. Hardcoded test-account credentials in SKILL.md (blocker)

- **Login email:** umar.kabaili@yopmail.com
+ **Login email:** azam.sawati@yomail.com 
   **Login password:** Umar@123!@#

The password Umar@123!@# is committed in plain text in SKILL.md. Even if it's a throwaway yopmail account, hardcoding credentials in source is a bad habit that could get copied to real accounts. Move credentials to env vars (e.g. CONFIGURED_EMAIL / CONFIGURED_PASSWORD) and reference them from SKILL.md with a note to set the vars in .env.

2. wrongSituation index is 1-based but sit check is also 1-based — verify the off-by-one

wrongSituation = 1 + Math.floor(Math.random() * knownCount); // 1-based
// ...
const makeWrong = sit === wrongSituation && !!correctText;

sit starts at 0 and is incremented before use (sit++ in the while), so it is 1-based when evaluated in the loop body. This is correct — but the pattern is fragile. Consider naming the variable wrongSituationIndex and adding a comment confirming both are 1-based to avoid future confusion.

3. Missing newline at end of baseline-completion.json

-}
\ No newline at end of file
+}

The file still ends without a trailing newline (the diff shows \ No newline at end of file on the new entry). Minor but easy to fix and avoids noisy diffs in future PRs.

4. setupToastObserver called before login in non---fresh path — document the dependency
In baseline-agent.mjs, setupToastObserver(page) is now called before login(). If setupToastObserver assumes an authenticated page state this could silently fail. The fix looks intentional (to catch toasts during a full flow that wasn't previously triggered here), but worth a comment explaining why it's needed before login.

5. E2E confirmation still pending for the answer-key fix
LEARNING.md itself notes: "end-to-end confirmation pending the next training run (key was truncated this run, so picks were blind)." The fix is logically sound, but the PR merges code changes that haven't been verified end-to-end yet. If possible, run one training pass with the new net-monitor and attach evidence before merging to staging.


Minor nits

  • The trailing space on line +- **Login email:** azam.sawati@yomail.com in SKILL.md should be removed.
  • WRONG_PROB default is 0.5 — with 6 units that means on average 3 units are candidates for the wrong-answer injection but only one wins (due to wrongInjected). The probability is fine; just make sure the SKILL.md doc matches the implementation (it says "randomly chosen unit" which is accurate).

Verdict

Comment (not blocking merge, but address before next staging QA run). The core fixes are correct and the test-coverage feature is well-designed. The main ask is: (a) don't commit credentials to source, and (b) get one clean end-to-end training run as evidence before calling the answer-key fix done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants