[jira] [Commented] (HIVE-14536) Unit test code cleanup

["Peter Vary (JIRA)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/HIVE-14536?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15453943#comment-15453943 ] 

Peter Vary commented on HIVE-14536:
-----------------------------------

Hi [~kgyrtkirk], [~sseth],

I have to apologize here, since if everyone thought after HIVE-14444, that we should refactor these classes only after the dust is settled, it was clearly my lack of open source experience which caused problem here. You know, if everyone travels on a highway opposite to me, clearly I must have chosen a wrong lane :)
To explain myself a little more, in my previous (numerous closed source) projects, we usually finalized the basic concepts before adding new features, non critical fixes and tuning performance, but starting to learn, that in open source the "many baby steps" is a way to go. 

Anyway, sorry for the misunderstanding again! I do my best, and learning as fast as I can! :)

Some words for the changes in the patch:
I think there will be the following typical use cases for the qtest infrastructure:
- Adding a new query - No differences here between the two solutions
- Adding a new test configuration
-- With the current solution we have to change minimum 2 files CliConfigs, TestCase, and probably a driver
-- After the patch these changes are in one place, the TestCase
- Looking for a problem with a particular TestCase
-- With the current solution we go to the TestCase, follow the buildClassRule method, realize, that it is defined by the Configuration, go to the CliConfigurations, find the specific Adapter, find the method, and follow it to the QTestUtil implementation - which means 4 redirection
-- After the patch from the TestCase you could go directly to the QTestUtil implementation - immediate, straightforward
- Changing something in the QTestUtil, and checking which TestCases are effected by the change
-- With the current solution we will find the affected Adapter easily, but after that it is manual work, to find which TestCases are using that adapter
-- After the patch we could get direct link to the effected TestCases

While it is good to use the frameworks as such, but one have to be careful to use the features which are beneficial in the current situation. Here we absolutely need the parameterized test running, exactly as it has been implemented, but in my opinion using rules not helps in our case. I think there are many ok designs, like the current one, but our best solution would be the following:
- Configuration - storing the test configurations - same as currently
- ConfigBuilder - creating the test configurations - as both of you pointed out, and I implemented it
- TestCase - Defining the setup/before/after/shutdown functionality, especially since some of the test cases need specific setup/cleanup procedures
- TestUtil(s) - Encapsulating the reoccurring functionality - for example there are 4 kinds of test running and result evaluating methods used across the TestCases, there is a typical functionality to remove created tables etc.

With this in mind, I still think we would have better, more maintainable solution after this patch.

I hope I answered all your questions and comments regarding this patch. If not so, or you do not agree with me, just write. I really hope, that I did not blocked all work on the test cases - I have merged every affected change to this one, and I am ready to do so with the following changes too, like HIVE-14532, which I have already reviewed. Seriously! :) So do not hesitate to work on them.

If we agree, that the general direction is good, and the patch needs only incremental changes like the ConfigBuilder, I would be happy to invest the time and the effort to make it happen. If you do not agree even with the basic direction then please state it.

Thanks, and apologies again,
Peter


> Unit test code cleanup
> ----------------------
>
>                 Key: HIVE-14536
>                 URL: https://issues.apache.org/jira/browse/HIVE-14536
>             Project: Hive
>          Issue Type: Sub-task
>          Components: Testing Infrastructure
>            Reporter: Peter Vary
>            Assignee: Peter Vary
>         Attachments: HIVE-14536.5.patch, HIVE-14536.6.patch, HIVE-14536.7.patch, HIVE-14536.8.patch, HIVE-14536.patch
>
>
> Clean up the itest infrastructure, to create a readable, easy to understand code



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)