Uh oh!
There was an error while loading. Please reload this page.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
program.cppwas removed from linking unconditionally. That fixes duplicate-symbol errors when tests#include "program.cpp", but it breaks extern-style tests (shown in README.md (line 104) and codeeval.http (line 38)) because student symbols are no longer linked.Can we:
testCodewill contain#include program.cpp?program.cppand prepend them intotest_program.h. I think this might be injecting bad declarations and giving back wrong return types seen in @murthy-001's screenshot in Slack https://codewitus.slack.com/files/U08SNJSLWKU/F0AFCDLJ6PR/screenshot_2026-02-18_at_3.44.38___pm.png.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From a strategic standpoint, it's important that we let the user submit code that matches the video's code exactly and it should always pass the tests.
A secondary priority is that we want to avoid the system having to edit the user's submitted attempt (such as wrapping it in a novel
namespace, deleting some code, or other workarounds) mostly because (A) it'd add more complexity and potential points of failure, and (B) compilation errors would likely report inconsistent line numbers with what the user actually submitted.Therefore, it makes sense to put the burden on the test code for every exercise to adapt as necessary to test what was demonstrated in the video.
Yes. I'll make sure to document it for reference purposes, but this seems like our best solution. The test code will need to manipulate how
program.cppis processed (e.g. capture cin/cout to automate simulated console interaction, redefinemainasprogram_main, etc.) so we'll need to adopt the practice of making sure to always#include program.cppin the test filetest_program.hI've verified that the approach this PR takes fixes the compilation error when run manually within the codewit-us/containership/Dockerfile.cpp.box container. However, before merging it, we should verify that it works in local development with codeval + codewit.us
Agreed, I'll update this PR with those revisions as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR #13 fixes the immediate duplicate-linking failure for include-style C++ tests by removing
program.cppfrom the runner link step, and I have confirmed that include-style payloads now pass locally.Approving this as a scoped hotfix, with one suggested follow-up issue before/alongside rollout:
executor.js(the regex-generated declarations prepended totest_program.h) so test behavior is owned entirely bytestCode(#include "program.cpp,mainrenaming, I/O adaptation), since the current injection can still introduce signature conflicts (e.g., unsigned int vs int) and cause compile errors unrelated to student correctness.