Skip to content
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.

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

Testcase judgehost naming #2211

Closed
wants to merge 4 commits into from
Closed

Conversation

vmcj
Copy link
Member

@vmcj vmcj commented Nov 8, 2023

fix #2059

@eldering is this what you wanted in the issue? I'm not sure how/where to document the 2nd commit and if we want it.

@vmcj vmcj requested a review from eldering November 8, 2023 21:16
if ($testcaseFromRequest) {
$tc = $this->em->getRepository(Testcase::class)->find($testcaseFromRequest);
if ($tc) {
return $this->redirectToRoute('jury_problem_testcases', ['probId' => $tc->getProblem()->getProbId()]);
Copy link
Member

Choose a reason for hiding this comment

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

I have the impression that this behavior can be very confusing. You are requesting one problem with a testcase from a different problem and are redirected?

Perhaps it becomes less confusing if we just add a separate route for it (in case it is useful)?

Copy link
Member Author

Choose a reason for hiding this comment

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

So currently it is undocumented behaviour, and I think there are moments where you might want to know what testcase belongs with which problem (there is on the judgehost a system problem and you want to findout what kind of behaviour that problem has (loads of IO, loads of memory etc.)

I'm fine with just removing the commit, or just making a new route.

@meisterT
Copy link
Member

A screenshot would help for the second commit.

@vmcj vmcj force-pushed the testcase_judgehost_naming branch from 0c3dde0 to ec52400 Compare November 24, 2023 11:02
@@ -62,6 +63,7 @@
title="Move testcase down"><i class="fas fa-arrow-down"></i></a>
{% endif %}
</td>
<td rowspan="2">{{ "%05d"|format(testcase.testcaseid) }}</td>
Copy link
Member

Choose a reason for hiding this comment

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

It would be nice to add a more complete path if it fits in the UI.

vmcj added 3 commits November 27, 2023 14:19
Needed for DOMjudge#2059, this now
shows besides the rank (order of execution) also the ID in the database
as these can be in different order. On the filesystem we have a form of
`/2/2/testcase001` where this can now be checked against the UI.
With this we can now use:
http://localhost/jury/problems/2/testcases?testcase=1

with the example problems and get redirected to problem 1 (hello world)
as that has the first testcase. With this we have a shortcut to get from
the judgehost directory to something in the UI.
This reverts commit 337bb6e.

After discussion we didn't have a concrete usecase for this (yet).
@vmcj
Copy link
Member Author

vmcj commented Nov 27, 2023

A screenshot would help for the second commit.

image

And when you hover you get an educated guess for the absolute path. Educated as you can configure something else on the judgehost versus what you did on the domserver.

@vmcj vmcj force-pushed the testcase_judgehost_naming branch from ec52400 to 1adf8e5 Compare November 27, 2023 21:03
When someone hovers over the relateive path they get an educated guess
for the full path. If someone would use a different configure script (or
other options) we can't guess the path on the domserver as the
judgehost can work with different paths (prefix=$HOME versus
prefix=/opt). For those users who did this they will be able to work
this out from the absolute path.
@eldering
Copy link
Member

I think #2324 is a better solution: both in the place/way the information is displayed and also by taking the actual directory from the judgehost instead of depending on configure/build time variables.

@vmcj vmcj closed this Feb 23, 2024
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.

Testcase in output/judgings/... directory is not easily matched with web UI
4 participants