sqoop-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Dian Fu" <dian0511...@gmail.com>
Subject Re: Review Request 39817: Sqoop2: Add tests for ConfigFiller
Date Tue, 03 Nov 2015 02:44:32 GMT


> On Nov. 2, 2015, 11:09 p.m., Jarek Cecho wrote:
> > shell/src/main/java/org/apache/sqoop/shell/utils/ConfigFiller.java, lines 65-70
> > <https://reviews.apache.org/r/39817/diff/3/?file=1113822#file1113822line65>
> >
> >     I'm wondering what is the purpose of the test flag? Can we drop it?
> >     
> >     I would much rather to test the class without any special flags to get as close
to "production" as possible.
> 
> Dian Fu wrote:
>     In methods like fillInput...WithBundle, reader.putString(String str) is called. This
method will write the specified string into the internal buffer of ConsoleReader. As we have
already filled the internal buffer of ConsoleReader with our test data, the writen string
by reader.putString() will be mixed with the test data. I agree we should test as close to
"production" as possible. But the functionality of reader.putString(String str) is just to
display some string on the console and so disabling it during test won't make the test much
different from "production".
> 
> Jarek Cecho wrote:
>     I think that I've seen the problem you're describing when I played with the patch.
What I've seen is that some of the tests are reusing single Input multiple times and if if
the reused one alredy had a value, the it got "messed up". I do however feel that it's a test
problem because that is an expected behavior. If the input have a value, then user is editing
it rather then inserting it from scratch.
>     
>     I do feel that the right solution here is to either not reuse the inputs or call
setEmpty() before each test rather then conditionally skip valied pieces of code.
> 
> Dian Fu wrote:
>     This solution still has problems. For example, for method fillInputStringWithBundle,
if we set the maximum length of the string to 30 and then we provide an input with length
exceeds 30 at the first time of input. As the length exceeds the maxinum length, it will recursively
call itself to let user give a new input. But at this time, as the Input already has a value
and so method reader.putString() will still be called.

PS: In the new patch, we already call setEmpty() before each test.


- Dian


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39817/#review104815
-----------------------------------------------------------


On Nov. 3, 2015, 1:54 a.m., Dian Fu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39817/
> -----------------------------------------------------------
> 
> (Updated Nov. 3, 2015, 1:54 a.m.)
> 
> 
> Review request for Sqoop.
> 
> 
> Bugs: SQOOP-2648
>     https://issues.apache.org/jira/browse/SQOOP-2648
> 
> 
> Repository: sqoop-sqoop2
> 
> 
> Description
> -------
> 
> This JIRA add tests for ConfigFiller. ConfigFiller is used by interactive mode.
> 
> 
> Diffs
> -----
> 
>   shell/src/main/java/org/apache/sqoop/shell/utils/ConfigFiller.java 6a2a96d 
>   shell/src/test/java/org/apache/sqoop/shell/utils/TestConfigFiller.java PRE-CREATION

> 
> Diff: https://reviews.apache.org/r/39817/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Dian Fu
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message