-
Notifications
You must be signed in to change notification settings - Fork 23
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
IDoCT Evaluation Task #27
Conversation
IDoCT Evaluation Task
@rupeshkoushik07 Thank you for the pull request! I have reviewed it and have several comments.
Please let me know if you need any further clarification! |
@Alex-Lian Thank you for you review. |
Yes, the grading scripts may not work properly if you do not insert the results at the end of the file. I manually checked the tests you selected and they looks good to me. I have one more comment: |
Yes, I completely agree. The value -1 is a bad value and yet passes the test. I have updated it |
@rupeshkoushik07 I have reviewd your update and found one more problem that should be addressed. Line 1812-1833 are from your pull request. You can see clearly from the picture that the separators between the items in each row are not uniform, which should be `\t`. You can refer to line 1811 to revise your pull request.By the way, we also provide the scripts that you can use to check your pull request. |
@rupeshkoushik07 The CI tests fail, Could you fix the PR to pass the CI? |
The CI scripts are the same as pointed by @Alex-Lian in the early comment, |
@tianyin The result of CI scripts show that the test name is repeated. This appears to be a false alarm caused by the new results being improperly inserted in the first commit of this pull request. The current pull request looks good to me. We can merge it now. Let me adjust the |
@Alex-Lian can you merge it in that case? Thank you for working on the PR @rupeshkoushik07 -- we will follow up on other stuff via emails. |
@Alex-Lian hmmm if you think it's a common error, we may want to keep it as long as we can identify it quickly -- the PR needs to be reviewed anyway. |
@rupeshkoushik07 LGTM. I will merge the PR now. |
@tianyin I reviewed the onboarding task that we wrote last year. We ask students to choose five random tests using the command Considering the possibility of students selecting tests that have already been chosen by others, I think using a warning instead of a failure would be a more reasonable approach. This way, even if one or two tests overlap, it would be considered acceptable and not impede the completion of the task. Or we can modify the onboarding task to ask students to choose untouched tests. |
Can you open a new issue to discuss this, instead of in this PR? I think it's a good one to discuss. |
please feel free to refer to this PR. |
Ok, I will write the issue now. |
Description of PR
I conducted CTests with three different valid values and at least one invalid value to assess the correctness and effectiveness of the CTest.
CTest1 : org.apache.hadoop.net.TestScriptBasedMapping#testFilenameMeansMultiSwitch
Parameter :net.topology.script.number.args
values:
512 GOOD PASS
1024 GOOD PASS
10000 GOOD PASS
Abc BAD FAIL
CTest2 : org.apache.hadoop.security.TestWhitelistBasedResolver#testFixedVariableAndLocalWhiteList
Parameter : hadoop.rpc.protection
values
abc BAD FAIL
integrity GOOD PASS
authentication GOOD PASS
privacy GOOD PASS
CTest3: org.apache.hadoop.conf.TestConfiguration#testUpdateSocketAddress
Paramter1: hadoop.security.dns.log-slow-lookups.threshold.ms
values
0 GOOD PASS
-1 GOOD PASS
215 GOOD PASS
31415926535 BAD FAIL
Prameter2: hadoop.security.dns.log-slow-lookups.enabled
values
TRUE GOOD PASS
FALSE GOOD PASS
CTest4:org.apache.hadoop.io.compress.TestCompressionStreamReuse#testGzipCompressStreamReuseWithParam
Parameter: io.file.buffer.size
values
0 BAD FAIL
512 GOOD PASS
256 GOOD PASS
1024 GOOD PASS
CTest5:org.apache.hadoop.net.TestNetUtils#testWrapSocketException
Parameter: hadoop.security.dns.log-slow-lookups.threshold.ms
values
Abc BAD FAIL
220 GOOD PASS
100 GOOD PASS
5000 GOOD PASS