QNTM-1476: Upload JSON files to S3 by smangarole · Pull Request #8136 · DynamoDS/Dynamo

smangarole · 2017-08-28T21:32:59Z

Please Note:

Before submitting the PR, please review How to Contribute to Dynamo
Dynamo Team will meet 1x a month to review PRs found on Github (Issues will be handled separately)
PRs will be reviewed from oldest to newest
If a reviewed PR requires changes by the owner, the owner of the PR has 30 days to respond. If the PR has seen no activity by the next session, it will be either closed by the team or depending on its utility will be taken over by someone on the team
PRs should use either Dynamo's default PR template or one of these other template options in order to be considered for review.
PRs that do not have one of the Dynamo PR templates completely filled out with all declarations satisfied will not be reviewed by the Dynamo team.
PRs made to the DynamoRevit repo will need to be cherry-picked into all the DynamoRevit Release branches that Dynamo supports. Contributors will be responsible for cherry-picking their reviewed commits to the other branches after a LGTM label is added to the PR.

Purpose

The purpose of this PR is to Upload newly generated JSON files to S3. In this PR, I updated the TestFixtureSetup() to delete the files in the temp folder. This will enable us to upload a new set of files to s3 after every test run. This PR is a part of the testing tool that we are building

Declarations

Check these if you believe they are true

The code base is in a better state after this PR
Is documented according to the standards
The level of testing this PR includes is appropriate
All tests pass using the self-service CI.
(https://github.com/DynamoDS/Dynamo/wiki/Dynamo-Versions), and are documented in the API Changes document.

Reviewers

@mjkkirschner @ramramps @QilongTang

mjkkirschner · 2017-08-28T22:54:44Z

test/DynamoCoreTests/SerializationTests.cs

+            var jsonFolder = Path.Combine(tempPath, "json");
+            var jsonNonGuidFolder = Path.Combine(tempPath, "jsonNonGuid");
+
+            var jsonNonGuidFiles = Directory.GetFiles(jsonNonGuidFolder, "*.*", SearchOption.AllDirectories);


you need to either verify these paths exist or put these calls inside a try catch, Directory.GetFiles will throw if the directories don't exist... which they may not on a clean machine.

mjkkirschner · 2017-08-28T22:55:31Z

test/DynamoCoreTests/SerializationTests.cs

+        {
+            foreach (var file in files)
+            {
+                File.Delete(file);


this should probably be made more tolerant, assert that the files actually exist before you try this and check if the deletion succeeds, you may not have permissions when this call is made.

mjkkirschner · 2017-08-28T22:56:32Z

test/DynamoCoreTests/SerializationTests.cs

            modelsGuidToIdMap.Clear();
            DoWorkspaceOpenAndCompare(filePath, "json_nonGuidIds", ConvertCurrentWorkspaceToNonGuidJsonAndSave, CompareWorkspacesDifferentGuids, SaveWorkspaceComparisonDataWithNonGuidIds);
+
+            //UploadToS3() from Ram;


you will want to upload both the guid and non guid tests and all data files associated with those graphs so this should be called from the other serializationTest method as well.

hmm, you may want to add a breakpoint here and check if this method is called repeatedly... I dont think you want to make 2000 separate upload calls to s3, I think you just want to upload the two folders once.

If Dynamo is going to upload to S3, then the configurations should be moved to the config files or environment variables. Uploading to S3 is "sync" ing a folder on the machine with S3 storage location. So, one sync will get all the files in S3.

@ramramps @smangarole thats a good point... I dont think running these tests should upload to s3 - then what happens when a dev runs the test locally? It should not upload to s3. this should be some post test step or at least set with a flag or something.

Please make uploading part seperate of Dynamo code. For internal tool, we probably can argue about it. But this will be wrapped in DynamoTest nuget package and spread publicly.

another thought here is upload should be from the script (job) after a successful build.

gregmarr · 2017-08-30T14:52:43Z

test/DynamoCoreTests/SerializationTests.cs

+
+                try
+                {
+                    jsonFilesList = Directory.GetFiles(jsonNonGuidFolder, "*.*", SearchOption.AllDirectories).ToList();


if jsonFolder exists but jsonNonGuidFolder does not, this will prevent deletion of the jsonFolder files. These should probably be in separate try/catch blocks.
Can you use Directory.Delete(jsonNonGuidFolder, true) here instead of deleting the files individually?

agree with @gregmarr 👍

The current "delete files individually" method has the problem that as soon as you try to delete a file for which you don't have delete permission (or it's currently locked), then it aborts the entire operation.

added individual try...catch for each folder.

gregmarr · 2017-08-30T14:56:32Z

test/DynamoCoreTests/SerializationTests.cs

+            foreach (var file in files)
+            {
+                //Delete files if they exist
+                if (File.Exists(file))


This check isn't necessary (and the entire function may not be necessary if Directory.Delete works). If the file didn't exist, it wouldn't be in the list. If it did exist when the list was generated, but doesn't now, then it's still safe to call File.Delete on it, as deleting a non-existent file doesn't throw an exception.

I agree. If I am using Directory.Delete, I can remove this method entirely.

gregmarr · 2017-08-30T14:57:01Z

test/DynamoCoreTests/SerializationTests.cs

        {
            modelsGuidToIdMap.Clear();
-            DoWorkspaceOpenAndCompare(filePath, "json_nonGuidIds", ConvertCurrentWorkspaceToNonGuidJsonAndSave, CompareWorkspacesDifferentGuids, SaveWorkspaceComparisonDataWithNonGuidIds);
+            DoWorkspaceOpenAndCompare(filePath, "json_nonGuidIds", ConvertCurrentWorkspaceToNonGuidJsonAndSave, CompareWorkspacesDifferentGuids, SaveWorkspaceComparisonDataWithNonGuidIds);        


looks like a trailing whitespace only change

gregmarr · 2017-08-30T17:56:00Z

test/DynamoCoreTests/SerializationTests.cs

+            var jsonNonGuidFolder = Path.Combine(tempPath, "jsonNonGuid");
+
+            var jsonFilesList = new List<string>();
+            var jsonNonGuidFilesList = new List<string>();


These two variables are now obsolete.

Oh, yes. Deleted!

Siddhartha Mangarole added 2 commits August 28, 2017 17:24

QNTM-1476: Update unit tests to delete old files from Temp folders

e83d86e

QNTM-1476: Specific extension not needed

6f6988b

smangarole requested review from QilongTang, mjkkirschner and ramramps August 28, 2017 21:33

mjkkirschner reviewed Aug 28, 2017

View reviewed changes

mjkkirschner added the Requires Changes label Aug 28, 2017

Siddhartha Mangarole added 3 commits August 30, 2017 10:00

Reviews

4e4e083

Update

6f8f72c

Comments

619551b

gregmarr reviewed Aug 30, 2017

View reviewed changes

Siddhartha Mangarole added 2 commits August 30, 2017 13:41

More reviews

589db38

Trailing white space removal

21e77e1

gregmarr reviewed Aug 30, 2017

View reviewed changes

Delete obsolete variables

301b375

QilongTang added LGTM and removed Requires Changes labels Sep 5, 2017

smangarole changed the title ~~WIP: QNTM-1476: Upload JSON files to S3~~ QNTM-1476: Upload JSON files to S3 Sep 6, 2017

smangarole merged commit fe43e7a into master Sep 6, 2017

QilongTang deleted the QNTM-1476 branch September 25, 2017 13:40

Conversation

smangarole commented Aug 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Purpose

Declarations

Reviewers

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

QilongTang Aug 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

smangarole commented Aug 28, 2017 •

edited

Loading

QilongTang Aug 29, 2017 •

edited

Loading