Skip to content

[FIX]: Guard against empty output file path in ServerComparer #14

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
x15sr71 wants to merge 1 commit into
base: master
Choose a base branch
from
+12 −1
Open

[FIX]: Guard against empty output file path in ServerComparer #14

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 12 additions & 1 deletion CCExtractorTester/Comparers/ServerComparer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,17 @@ public void CompareAndAddToResult(CompareData data)
// Check for equality by hash
if (!data.Dummy && !Hasher.filesAreEqual(data.CorrectFile, data.ProducedFile))
{
// Guard: if no output file was produced, skip upload.
// When ProducedFile is empty (output missing), File.Open("") throws
// ArgumentException which is swallowed by the caller's catch-all
// handler, resulting in no TestResultFile row being written.
// This prevents the ArgumentException from propagating to the
// caller's catch block.
if (string.IsNullOrEmpty(data.ProducedFile) || !File.Exists(data.ProducedFile))
{
return;

@canihavesomecoffee canihavesomecoffee Jun 14, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If you return without posting a status, how is the platform supposed to know something went wrong?

@x15sr71 x15sr71 Jun 15, 2026 •
edited
Loading

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

The finish POST (exit code + runtime) goes through the same as before this PR, SendExitCodeAndRuntime runs after the comparison loop, outside the try/catch, so neither the old swallowed exception nor the new early return affects it. The TestResult row is created either way; only the equality/upload POST is skipped, so no TestResultFile row is written for that output.

The platform reads that absence as a failure. In get_test_results() (mod_test/controllers.py), when a regression test has no TestResultFile rows it checks RegressionTestOutput directly and sets test_error = True if a non-ignored output was expected (the else branch). That runs on every status surface that calls it, PR status (via comment_pr), PR comment, badge, detail page, except commit-test status, which still uses the old count-based query in progress_type_request where zero rows reads as zero failures → SUCCESS. That's the actual bug; #1119 fixes it by moving that path onto get_test_results().

So this PR is the narrower part: it stops the silent ArgumentException and makes the missing-output path explicit, it doesn't change what's sent to the platform, since finish already fired and the comparison row was already absent before this change. One flag, the guard returns silently, so unlike the old swallowed exception (which hit processor.Logger.Error) there's no tester-side trace now. ServerComparer has no logger field; happy to add a Console.Error.WriteLine in the guard, or wire an ILogger into ServerComparer like Tester and Processor already use - whichever you prefer.

}

// Upload result
using (FileStream stream = File.Open(data.ProducedFile, FileMode.Open))
{
Expand Down Expand Up @@ -189,4 +200,4 @@ public void SendExitCodeAndRuntime(RunData rd, int testId)
}
}
}
}
}