Skip to content

Conversation

@ranaldmiao
Copy link
Contributor

@ranaldmiao ranaldmiao commented Mar 7, 2021

This implementation re-uses the same header files as that for actual submissions. (Assumption: Task authors typically do not have different implementations for public and private header files.)

  • This PR also allows for graders for user tests to be downloaded. Previously, a HTTP 404 will be encountered when contestants select the option to download grader files from their user tests.

It seems that graders are not stored with the .%l suffix (Eg: grader.%l) but the original extension (Eg: grader.cpp) instead. Hence, this PR introduces an additional check for the original filename before declaring HTTP 404.


This change is Reviewable

@ranaldmiao ranaldmiao changed the title Fix missing header files in user tests Fix missing misc issues with user tests with graders Mar 7, 2021
@ranaldmiao ranaldmiao changed the title Fix missing misc issues with user tests with graders Fix misc issues with user tests with graders Mar 7, 2021
@codecov
Copy link

codecov bot commented Mar 7, 2021

Codecov Report

Merging #1172 (0e34b78) into master (9236084) will decrease coverage by 0.13%.
The diff coverage is 33.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1172      +/-   ##
==========================================
- Coverage   63.63%   63.50%   -0.14%     
==========================================
  Files         232      232              
  Lines       17044    17048       +4     
==========================================
- Hits        10846    10826      -20     
- Misses       6198     6222      +24     
Flag Coverage Δ
functionaltests 47.42% <33.33%> (-0.15%) ⬇️
unittests 44.13% <0.00%> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
cms/server/contest/handlers/taskusertest.py 46.55% <0.00%> (ø)
cms/grading/Job.py 89.73% <50.00%> (-0.73%) ⬇️
cms/service/EvaluationService.py 71.39% <0.00%> (-2.74%) ⬇️
cms/server/contest/handlers/tasksubmission.py 48.63% <0.00%> (-2.19%) ⬇️
cms/db/base.py 87.12% <0.00%> (-1.99%) ⬇️
cms/service/esoperations.py 79.86% <0.00%> (-1.39%) ⬇️
cms/io/rpc.py 92.85% <0.00%> (-1.37%) ⬇️
cms/service/ProxyService.py 66.66% <0.00%> (-1.02%) ⬇️
cms/service/ResourceService.py 59.11% <0.00%> (-0.89%) ⬇️
cms/server/admin/handlers/base.py 70.29% <0.00%> (-0.67%) ⬇️
... and 5 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9236084...0e34b78. Read the comment docs.

@ranaldmiao ranaldmiao changed the title Fix misc issues with user tests with graders Fix misc issues regarding user tests with graders Mar 7, 2021
Copy link
Member

@andreyv andreyv left a comment

Choose a reason for hiding this comment

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

Thanks for the fixes!

The first change is related to TaskType.get_auto_managers(), which is supposed to return managers to be taken from the task. But you can see that currently there is no way for get_auto_managers() to account for a non-mandatory, arbitrarily named .h manager (even less so for additional .o managers). One could make it work now by returning None instead of [] from Batch.get_auto_managers(), but I don't think this is intended.

So I think that for now the proposed change is a sufficient short-term fix until a general solution is worked out.

The second change looks good. Managers are in general stored without .%l in the database, so it is fitting to do the same here.

@ranaldmiao
Copy link
Contributor Author

Thanks for the review, @andreyv

Made the requested changes.

@ranaldmiao ranaldmiao requested a review from andreyv March 20, 2021 04:39
@andreyv andreyv merged commit c279f81 into cms-dev:master Mar 26, 2021
@andreyv
Copy link
Member

andreyv commented Mar 26, 2021

Thanks, merged. Also added tests in 81fb9df.

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

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants