> 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.
>
> Dian Fu wrote:
> PS: In the new patch, we already call setEmpty() before each test.
I'm glad that the Input.setEmpty() worked for all test cases other then the
case in fillInputStringWithBundle with exceeding characters.
I was thinking about it a bit and I think that this is expected behavior. If
user specifies too long string input the code will print the original value
back and ask the user to make it shorter. I think that we should test this
behavior rather then changing the behavior for just test purpose. I was able to
fix the test case by adding backspaces to the simulated user input (e.g. the
same what real user would do):
// Retry when the given input exceeds maximal allowance
String lengthExceeds30 =
"abcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyz";
String remove30characters =
"\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b";
String lengthBelowLimit = "abcdefg";
initData(lengthExceeds30 + "\r" + remove30characters + lengthBelowLimit +
"\r");
input.setEmpty();
assertTrue(fillInputStringWithBundle(input, reader, resourceBundle));
assertEquals(input.getValue(), lengthBelowLimit);
What do you think?
- Jarek
-----------------------------------------------------------
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
>
>
