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