Closed rupeshkoushik07 closed 1 year ago
@rupeshkoushik07 Thank you for the pull request! I have reviewed it and have several comments.
org.apache.hadoop.http.TestHttpServer#testAcceptorSelectorConfigurability
has already been chosen by another student (refer to lines 1764-1808). Could you please replace it with another test? You can use the following command to randomly select a test:
nl $idoct_dir/generate_mapping/results/hadoop-common/param_unset_getter_map.json | grep \# | shuf --random-source=<(while :; do echo {your email address}; done) | head
Please let me know if you need any further clarification!
@Alex-Lian Thank you for you review. Could you please point out the reference where I included the test org.apache.hadoop.http.TestHttpServer#testAcceptorSelectorConfigurability in my commit? .I believe I have'nt included the test.
@rupeshkoushik07
Could you please point out the reference where I included the test org.apache.hadoop.http.TestHttpServer#testAcceptorSelectorConfigurability in my commit? .I believe I have'nt included the test.
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:
On line 22, why do you think the value -1
is a GOOD
value? The description for this parameter is "If slow lookup logging is enabled, this threshold is used to decide if a lookup is considered slow enough to be logged." (Note that GOOD/BAD
does not depend on whether the ctests pass or fail.) Two interesting situations are: 1. GOOD value -> Ctest Fail. 2: BAD value -> Ctest Pass.
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, https://github.com/xlab-uiuc/IDoCT/actions/runs/5447938997/jobs/9910633245?pr=27
Could you fix the PR to pass the CI?
The CI scripts are the same as pointed by @Alex-Lian in the early comment, https://github.com/xlab-uiuc/IDoCT/pull/27#issuecomment-1615232110
@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 repeated ctest
as a warning instead of an error in the CI scriptsa after this pr is merged.
@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.
Let me adjust the repeated ctest as a warning instead of an error in the CI scriptsa after this pr is merged.
@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 find $hadoop_dir/hadoop-common-project/hadoop-common -name "*Test*java" | grep -v generated-test | shuf
.
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