search

xlab-uiuc / IDoCT

Illinois Dataset of Configuration Tests
3 stars 23 forks source link

[Add] Four rewritten tests based on hadoop-common #3

Closed Justin-Xiang closed 2 years ago

Justin-Xiang commented 2 years ago

Based on the information from Ctest and examples, I rewrite 4 tests which are all hardcoded tests in the hadoop-common project and generate 4 git patch files. After removing the set call, the parameter to be tested comes from system configuration, which means rewritten ctest can then test it.

I notice that there is no tsv file like test_rewrite.tsv in this repo. If ok, I'd like to make another PR to fulfill this work. :)

tianyin commented 2 years ago

I am so impressed!

We'll verify the correctness of your rewrite.

I notice that there is no tsv file like test_rewrite.tsv in this repo. If ok, I'd like to make another PR to fulfill this work.

Yes, please! Thank you!

Justin-Xiang commented 2 years ago

@tianyin Thanks! If the correctness of my rewrite is ok, I will make another PR to add a file like this. If not, I will check what's the problem with it and try to fix it as soon as possible. Look forward to your feedback and suggestions soon. :)

xylian86 commented 2 years ago

Incredible work! @Justin-Xiang I am checking the PR now (looks good, but I need to dig the code and test it)

Justin-Xiang commented 2 years ago

@Alex-Lian Thank you for your time and welcome any suggestions. :)

xylian86 commented 2 years ago

I tested the PR and worked well on my side. Btw, it is pretty wonderful that @Justin-Xiang chose four different test classes.

shuaiwang516 commented 2 years ago

Thanks for the PR @Justin-Xiang,

I checked your patches for the 4 tests and all of them are deleting hardcoded configuration (e.g., conf.set()) and explicit assertions (e.g., assertEquals()). This is good and it's also the simplest way of rewriting a ctest.

Besides this, you can also try to not just delete the assertions in the rewriting, but think about how to modify the assertions to test the configurations still.

For example this test: hadoop/test_rewrite/org.apache.hadoop.ipc.TestIdentityProviders. After deleting the line 18-20, the test can indeed pass with different value of CommonConfigurationKeys.IPC_IDENTITY_PROVIDER_KEY, but LOSES the part of testing IdentityProviders.getClass(). What we can do is to rewrite the line 20 to something like assertEquals(ip.getClass(), conf.get(CommonConfigurationKeys.IPC_IDENTITY_PROVIDER_KEY, IdentityProvider.class));. (This is only an example so I use conf.get() here, you may need to check what is the actual configuration API for getting CommonConfigurationKeys.IPC_IDENTITY_PROVIDER_KEY)

There are also some other tests that are even harder to rewrite (e.g., implicit assertions of configurations in tests). If you're interested, you can also try to find those and think about how to rewrite them.

Justin-Xiang commented 2 years ago

@shuaiwang516 Thanks a lot for this really useful advice. It let me think about rewriting tests from another perspective.

During the process of modification, what doubts me first is that ip.getClass() returns the runtime type of ip, and that refers to the CommonConfigurationKeys.IPC_IDENTITY_PROVIDER_KEY(e.g. "org.apache.hadoop.ipc.UserIdentityProvider" here) . Besides, IdentityProvider is an interface and IdentityProvider.getClass()here will face a compile-time error since it hasn't been declared.

In this case, I use Class.forName to return the Class object associated with the class with the given string name, then I use getName() to do this test work. You can get the details from my newest commit.

I'm currently also working on modifying the tests I have rewritten and finding ways to rewrite other tests that are harder than this one.

Looking forward to your feedback and suggestions. :)

shuaiwang516 commented 2 years ago

Great work! The new version LGTM! @Justin-Xiang

Looking forward to your other rewrites. Let me know if you want to discuss some hard cases or meet any problems.

tianyin commented 2 years ago

@shuaiwang516 could you merge it?

tianyin commented 2 years ago

@Justin-Xiang do you want to chat over Zoom in the next week or so? You passed our test and it's the time to discuss about the research plan.

shuaiwang516 commented 2 years ago

@shuaiwang516 could you merge it?

@Justin-Xiang is still working on the other 3 rewrites I think. I will merge this PR after he finishes them.

tianyin commented 2 years ago

If the current rewrites are done, let's merge incrementally, instead of waiting for a big PR (which is hard to review and have higher risk of conflicts).

shuaiwang516 commented 2 years ago

If the current rewrites are done, let's merge incrementally, instead of waiting for a big PR (which is hard to review and have higher risk of conflicts).

Agree. There are in total four rewrites in this PR and @Justin-Xiang has finished one of four based on my previous comment.

Justin-Xiang commented 2 years ago

@Justin-Xiang is still working on the other 3 rewrites I think. I will merge this PR after he finishes them.

@shuaiwang516 I have modified TestLoadBalancingKMSClientProvider and TestSwitchMapping. As for the former, there is no need to delete the other assertions which may lead to losing some tests, so I just delete the conf.set. For the latter, I rewrite it to get the scriptname from NET_TOPOLOGY_SCRIPT_FILE_NAME_KEY, which is more suitable. I think TestLdapGroupsMapping is ok and there is no need to modify it. I am looking forward to your feedback. :)

@Justin-Xiang do you want to chat over Zoom in the next week or so? You passed our test and it's the time to discuss about the research plan.

@tianyin Great! I'm excited about this! I'm in the UTC+8 timezone, which may be different from yours. Anytime for me is ok but I would appreciate it if I needn't stay up late to get this.