search

COOL-cohort / COOL

the source code of the COOL system
https://www.comp.nus.edu.sg/~dbsystem/cool/
Apache License 2.0
45 stars 16 forks source link

Add iceberg and csvLoader unittest #42

Closed qlinsey closed 2 years ago

qlinsey commented 2 years ago

added iceberg related tests

KimballCai commented 2 years ago

I have checked your code and there are several bugs. I have pushed a new commit to your repo and you can check it. If it is correct, please merge it in your repo.

Besides, please check all your commits and clean useless imports or codes that are not used. And please also check your output log.

KimballCai commented 2 years ago

Please check that you can compile the code correctly.

qlinsey commented 2 years ago

I removed csv loader related files (removed csvLoader unittest) following PR #39. Pushed changes to my remote branch.

KimballCai commented 2 years ago

Thanks. This repo runs smoothly on my machine, but please clean the updates that are not used in cool-core/src/test/java/com/nus/cool/core/model/CoolModelTest.java.

qlinsey commented 2 years ago

Should i close this pull request? thanks

KimballCai commented 2 years ago

Lingze @Zrealshadow has some other comments on your repo about the code format.

No need to manually close the PR, and we will merge your PR after your modification.

Thanks

qlinsey commented 2 years ago

Sure , I will take a look.

qlinsey commented 2 years ago

I added DataProvider for two test cases in above, pls check . not sure where else I should add. Thanks.

Zrealshadow commented 2 years ago

Thanks for your PR. The tests works in my side. However, there are some improvements. Firstly, I have to point that these tests is not automatic test. Actually, you should add some assert to compare the result you get and you expect. In your test, you just print these result and judge it manually. Secondly, I think you should separate the test logic part from test data input part completely for future scalability. For example, in you IcebergQueryTest , you set @BeforeMethod annotation in setUp method, which means this method will be invoked before every test method. But what you do in setUp is just setting some properties for one of your unit test. Obviously, if we add other test method, you have to change this part of code.

@KimballCai No error in these code, I think we can merge it. but it's not automatic test. these code should be refactored and improved in the future.

Zrealshadow commented 2 years ago

I added DataProvider for two test cases in above, pls check . not sure where else I should add. Thanks.

@qlinsey The modification in file CoolModelTest.java doesn't make sense. Delete this commit. Then, We can merge it.

qlinsey commented 2 years ago

I added DataProvider for two test cases in above, pls check . not sure where else I should add. Thanks.

@qlinsey The modification in file CoolModelTest.java doesn't make sense. Delete this commit. Then, We can merge it.

Questions: (1) I read CoolModelTest.java, is there any difference from CubeListTest() vs. what I wrote? Does it also require Assert for Array of String? For IcebergQueryTest, do you suggest me to add AssertEqual to json string? (2) I can move IcebergQuery initialization from setUp() to ToStringTest() method, please let me know. thanks. (3) For CoolModelTest.java, I think that is because I removed the test cases, some more blank lines added. Let me try to remove. Thanks

KimballCai commented 2 years ago

I added DataProvider for two test cases in above, pls check . not sure where else I should add. Thanks.

@qlinsey The modification in file CoolModelTest.java doesn't make sense. Delete this commit. Then, We can merge it.

Questions: (1) I read CoolModelTest.java, is there any difference from CubeListTest() vs. what I wrote?

please check https://github.com/COOL-cohort/COOL/pull/42/files There are several blank lines added. Just remove them.

Does it also require Assert for Array of String? For IcebergQueryTest, do you suggest me to add AssertEqual to json string? (2) I can move IcebergQuery initialization from setUp() to ToStringTest() method, please let me know. thanks. (3) For CoolModelTest.java, I think that is because I removed the test cases, some more blank lines added. Let me try to remove. Thanks

qlinsey commented 2 years ago

@KimballCai removed blank lines. thanks

KimballCai commented 2 years ago

Thanks for your PR. The tests works in my side. However, there are some improvements. Firstly, I have to point that these tests is not automatic test. Actually, you should add some assert to compare the result you get and you expect. In your test, you just print these result and judge it manually. Secondly, I think you should separate the test logic part from test data input part completely for future scalability. For example, in you IcebergQueryTest , you set @BeforeMethod annotation in setUp method, which means this method will be invoked before every test method. But what you do in setUp is just setting some properties for one of your unit test. Obviously, if we add other test method, you have to change this part of code.

@KimballCai No error in these code, I think we can merge it. but it's not automatic test. these code should be refactored and improved in the future.

noted

qlinsey commented 2 years ago

thanks