> 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.
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. - 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 > >
