nifi-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From trkurc <...@git.apache.org>
Subject [GitHub] nifi pull request: NiFI-1025: Imported aws-sdk:1.10.27 and joda-ti...
Date Thu, 05 Nov 2015 01:29:11 GMT
Github user trkurc commented on the pull request:

    https://github.com/apache/nifi/pull/107#issuecomment-153924808
  
    A couple of comments:
    
    I recommend changing S3Setup to something like AbstractS3Test and making it an abstract
class (if for nothing else, because it doesn't contain test code and is the base class for
the other tests)
    
    What this method did was a bit misleading. I expected that the parameter was the filename.
I'd rather see it with two parameters, key and filename, to make it a bit more general purpose.
If these tests are extended with multiple files, having that second parameter would be helpful.
Also, should it throw an AmazonS3Exception?
    
    ```
        protected void uploadTestFile(String key) throws IOException  {
    ```
    
    In the following code, rather than printing to System.err, would Asserting make more sense?
Or did you intend for the doesBucketExist be a catchall for these problems?
    ```
            } catch (final AmazonS3Exception e) {
                System.err.println("Can't create the key " + BUCKET_NAME + ":" + e.toString());
            } catch (final IOException e) {
                System.err.println(CREDENTIALS_FILE + " doesn't exist.");
            }
    
            if (!client.doesBucketExist(BUCKET_NAME)) {
                Assert.fail("Setup incomplete, tests will fail");
            }
    ```
    
    This one confused me a bit in TestDeleteS3Object.testSimpleDelete. I doesn't quite match
what the old testSimpleDelete method did, which was upload with key of hello.txt, with file
hello.txt. I think you're uploading with key "folder" and with file "/hello.txt". I reasonably
believe this does the same thing, but it was hard to read
    
    ```
        public void testSimpleDelete() throws IOException {
            // Prepares for this test
            uploadTestFile("folder");
    
            final TestRunner runner = TestRunners.newTestRunner(new DeleteS3Object());
    
            runner.setProperty(DeleteS3Object.CREDENTAILS_FILE, CREDENTIALS_FILE);
            runner.setProperty(DeleteS3Object.REGION, REGION);
            runner.setProperty(DeleteS3Object.BUCKET, BUCKET_NAME);
            runner.setProperty(DeleteS3Object.KEY, "folder");
    ```
    
    On a similar note I think TestDeleteS3Object.testSimpleDeleteFolder was intended to delete
with a key with a slash in it, as it was in the original test
    
    ```
        @Test
        public void testDeleteFolder() throws IOException {
            // Prepares for this test
            uploadTestFile("folder");
    ```
    
    I think that you may have propagated a bug from TestDeleteS3Object.testTryToDeleteNotExistingFile
[1]. I think the original test that second setting of BUCKET was erroneous. I think this should
be BUCKET_NAME instead of "no-such-a-key".
    
    ```
            runner.setProperty(DeleteS3Object.BUCKET, "no-such-a-key");
    ```
    
    Also, any thought to trying to exercise the Attribute Evaluation of the KEY in the tests?
All the KEY properties were set explicitly. 
    
    [1] https://github.com/apache/nifi/blob/master/nifi-nar-bundles/nifi-aws-bundle/nifi-aws-processors/src/test/java/org/apache/nifi/processors/aws/s3/TestDeleteS3Object.java#L122


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

Mime
View raw message