Un dev #63

uyennguyen16900 · 2020-07-25T05:42:59Z

doorknob/test_detect.py

All tests pass, except the first one.
I set the region to us-west-1, but I don't know why the regions for "SigningCertURL" and "UnsubscribeURL" are us-east-1.

In line 63 I compare acquired_message dict with expected dict.
Here is the output for print(acquired_message)

{
  "Message": "my message",
  "MessageId": "d118ecc5-6dc3-4b38-bb09-2514d6ae53eb",
  "Signature": "EXAMPLElDMXvB8r9R83tGoNn0ecwd5UjllzsvSvbItzfaMpN2nk5HVSw7XnOn/49IkxDKz8YrlH2qJXj2iZB0Zo2O71c4qQk1fMUDi3LGpi
j7RCW7AW9vYYsSqIKRnFS94ilu7NFhUzLiieYr4BKHpdTmdD6c0esKEYBpabxDSc=",
  "SignatureVersion": "1",
  "SigningCertURL": "https://sns.us-east-1.amazonaws.com/SimpleNotificationService-f3ecfb7224c7233fe7bb5f59f96de52f.pem",
  "Subject": "my subject",
  "Timestamp": "2020-07-24T12:00:00.000Z",
  "TopicArn": "arn:aws:sns:us-west-1:123456789012:some-topic",
  "Type": "Notification",
  "UnsubscribeURL": "https://sns.us-east-1.amazonaws.com/?Action=Unsubscribe&SubscriptionArn=arn:aws:sns:us-east-1:12345678
9012:some-topic:2bcfbf39-05c3-41de-beaa-fcfcc21c8f55"
}

Here is the output for print(expected)

{
  "Message": "my message",
  "MessageId": "19615f43-f2a4-416f-a6ee-3e02deb83fba",
  "Signature": "EXAMPLElDMXvB8r9R83tGoNn0ecwd5UjllzsvSvbItzfaMpN2nk5HVSw7XnOn/49IkxDKz8YrlH2qJXj2iZB0Zo2O71c4qQk1fMUDi3LGpi
j7RCW7AW9vYYsSqIKRnFS94ilu7NFhUzLiieYr4BKHpdTmdD6c0esKEYBpabxDSc=",
  "SignatureVersion": "1",
  "SigningCertURL": "https://sns.us-west-1.amazonaws.com/SimpleNotificationService-f3ecfb7224c7233fe7bb5f59f96de52f.pem",
  "Subject": "my subject",
  "Timestamp": "2020-07-24T12:00:00.000Z",
  "TopicArn": "arn:aws:sns:us-west-1:123456789012:some-topic",
  "Type": "Notification",
  "UnsubscribeURL": "https://sns.us-west-1.amazonaws.com/?Action=Unsubscribe&SubscriptionArn=arn:aws:sns:us-west-1:12345678
9012:some-topic:2bcfbf39-05c3-41de-beaa-fcfcc21c8f55"
}

…un-dev

UPstartDeveloper · 2020-07-25T14:43:32Z

.travis.yml

+<<<<<<< HEAD
+        - flake8
+=======
        - flake8 --exclude=doorknob/__init__.py,migrations
+>>>>>>> c7b73082a79eef302b7168ff3e80dc3c2293398b


@uyennguyen16900 , it looks like you changed the command for flake8 on your branch. Was the previous command (the one that says - flake8 --exclude=doorknob/__init__.py,migrations giving you errors?

In it's current state these 5 lines will cause a merge conflict, so if there are no errors using the 2nd command I think we should choose that over the basic - flake8, as it skips over certain files during testing that we don't need to worry about.

UPstartDeveloper · 2020-07-25T14:49:51Z

doorknob/foyer.py

-        aws_data: A list of detected face json objects generated by AWS
-                   Rekognition.
+        aws_data: A list of detected face json objects generated by AWS Rekognition.


I appreciate that you made the docstring look simpler @uyennguyen16900 . However this line 16 is a little long, it is 85 characters and the Pep 8 standard is to keep it below 80. You don't need to make it go onto the next line, but you please try and make the line length < 80 somehow?

UPstartDeveloper · 2020-07-25T14:55:17Z

doorknob/tests/test_foyer.py

@@ -1,8 +1,7 @@
 import json
 import unittest

-from doorknob.foyer import Scene
-
+from foyer import Scene


Importing from a higher level package in Python 3 is pretty tricky, and when I ran pytest on my machine this line caused a ModuleNotFoundError. However, could you try switching this line to from doorknob.foyer import Scene. Then, please verify with pytest that all the errors are resolved.

I had no module error because they are not in the same package. That's why I moved test_detect.py to its parent folder.

UPstartDeveloper · 2020-07-25T15:31:08Z

doorknob/test_detect.py

+from freezegun import freeze_time
+from detect import VideoDetect
+
+from moto import mock_sns, mock_sqs


The imports in these lines are also causing ModuleNotFound errors. Would you please make sure to freeze your requirements on requirements/local.txt and requirements/production.txt files if you installed new packages?

UPstartDeveloper · 2020-07-25T15:33:40Z

watch/tests.py

-from django.test import Client, TestCase
+from django.test import TestCase, Client
 from django.test.client import RequestFactory
-from watch.views import HomeView
+from django.urls import reverse, reverse_lazy
+from .views import (
+    HomeView,
+    WatchedView,
+)
+from django.core.files.uploadedfile import SimpleUploadedFile


Run isort watch/tests.py to double check the order of your imports is correct.

UPstartDeveloper · 2020-07-25T15:37:23Z

tinydoor/templates/pages/home.html

@@ -34,6 +45,7 @@ <h1 class="display-4">Measure User Satisfaction</h1>
    </div>
    <!--end of container-->
  <!--end of section-->
+>>>>>>> c7b73082a79eef302b7168ff3e80dc3c2293398b


Lines 11-48 shows that we have another merge conflict. When I ran this on localhost, the upload button on your branch is not as stylish as the one on the master.

Did you experience any error with the version of the upload button currently on master @uyennguyen16900? I think if it's possible we should just keep the upload button as is, as it will look better.

I'm sure why we have a merge conflict here since I haven't changed anything in html files.

Oh it's no problem. The conflict is probably because you just might not have pulled from the master branch in a while, because I'm assuming this branch was created a while back, before we integrated the Bootstrap theme into the HTML.

UPstartDeveloper · 2020-07-25T18:21:35Z

doorknob/test_detect.py

+
+        expected = MESSAGE_FROM_SQS % (message, published_message_id, "us-west-1")
+        acquired_message = re.sub(
+            "\d{4}-\d{2}-\d{2}T\d{2}:\d{2}:\d{2}\.\d{3}Z",


Interesting thing I learned about about Regular Expressions:

It is considered best practice to prepend your regex with an "r" - this helps the linter from throwing weird errors, such as thinking the "\d" is an escape sequence or something like that.

You can read more about that on this page from Flake8 Rules.

Un dev edits

UPstartDeveloper · 2020-07-25T23:17:45Z

doorknob/tests/test_foyer.py

+<<<<<<< HEAD
+=======
+
+>>>>>>> 7947f58b3010bd6e94e4351e2d33a23f81608875


@uyennguyen16900 please delete lines 5, 6, and 8. The git diff markers need to be removed, as they will cause the tests to fail.

UPstartDeveloper · 2020-07-25T23:23:02Z

doorknob/tests/test_foyer.py

@@ -16,6 +20,7 @@ def test_no_emotions(self):
            with self.assertRaises(ValueError):
                Scene(aws_data=json.load(f))

+
 class VideoDetectTestCase(unittest.TestCase):
    """


You can delete this class declaration now, since I see that you already have the tests for VideoDetect in another file.

I ran the SceneTestCase test class and they failed.

UPstartDeveloper

Hey @uyennguyen16900 - this PR is awesome.

Only some minor changes are needed, see my comments to help you because they mainly have to do with formatting.

Also, be sure to run black . at the top level of your local repo, so it can auto-fix some formatting errors. The command flake8 --exclude=migrations,env will also help highlight these for you.

Right now the main problem with the tests is in test_tasks.py, on line 7 when you call the reverse() with "run_task" as the argument. My question is: is the reverse function being used supposed to come from Django, or does it come from another library? Because right now the test thinks the argument is supposed to be the name of a view, and it fails because it can't find it.

Kudos for learning the moto package all by yourself!

uyennguyen16900 · 2020-07-25T23:39:08Z

Hey @uyennguyen16900 - this PR is awesome.

Only some minor changes are needed, see my comments to help you because they mainly have to do with formatting.

Also, be sure to run black . at the top level of your local repo, so it can auto-fix some formatting errors. The command flake8 --exclude=migrations,env will also help highlight these for you.

Right now the main problem with the tests is in test_tasks.py, on line 7 when you call the reverse() with "run_task" as the argument. My question is: is the reverse function being used supposed to come from Django, or does it come from another library? Because right now the test thinks the argument is supposed to be the name of a view, and it fails because it can't find it.

Kudos for learning the moto package all by yourself!

test_task.py is not working right now. I'm still playing around with it. My apologies for pushing incomplete tests. I had to commit everything I was working on in order to pull from master.

UPstartDeveloper · 2020-07-25T23:55:18Z

Oh no you're good! I was just curious in case the moto package had its own reverse function or something.

Code review is a good learning opportunity for both people.

uyennguyen16900 added 6 commits July 7, 2020 11:32

added unit tests

4bd7296

added kinesis

a112263

Resolved merge conflict

97baa47

added unittests for sns and sqs

dd45c90

Merge branch 'master' of https://github.com/escofresco/tinydoor into …

07f079b

…un-dev

added unittests for sns and sqs

c29e647

uyennguyen16900 requested a review from UPstartDeveloper July 25, 2020 05:43

deleted unwanted file

fc29372

UPstartDeveloper reviewed Jul 25, 2020

View reviewed changes

UPstartDeveloper self-assigned this Jul 25, 2020

UPstartDeveloper added the bug Something isn't working label Jul 25, 2020

UPstartDeveloper added this to In progress in Engineering Sprints via automation Jul 25, 2020

UPstartDeveloper added this to the User Dashboard milestone Jul 25, 2020

UPstartDeveloper linked an issue Jul 25, 2020 that may be closed by this pull request

Add Unit Testing Coverage #53

Open

UPstartDeveloper reviewed Jul 25, 2020

View reviewed changes

UPstartDeveloper added 2 commits July 25, 2020 14:11

Small formatting and requirements fixes.

2ec4e84

List moto and freezegun as dependencies, fix ImportError.

cbc861b

UPstartDeveloper reviewed Jul 25, 2020

View reviewed changes

Fix line length and unexpected escape sequence errors.

cae7f66

UPstartDeveloper marked this pull request as ready for review July 25, 2020 19:03

UPstartDeveloper and others added 4 commits July 25, 2020 15:21

Merge pull request #64 from escofresco/un-dev-edits

7947f58

Un dev edits

fixed syntax error

982cf62

fixed syntax error

52b650b

finish tests for sns and sqs

595840a

UPstartDeveloper reviewed Jul 25, 2020

View reviewed changes

deleted conflict markers

74afe31

UPstartDeveloper reviewed Jul 25, 2020

View reviewed changes

UPstartDeveloper requested changes Jul 25, 2020

View reviewed changes

Engineering Sprints automation moved this from In progress to Review in progress Jul 25, 2020

Merge branch 'master' into un-dev

c4152b3

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Un dev #63

Un dev #63

uyennguyen16900 commented Jul 25, 2020 •

edited

Loading

UPstartDeveloper Jul 25, 2020

UPstartDeveloper Jul 25, 2020

UPstartDeveloper Jul 25, 2020

uyennguyen16900 Jul 25, 2020 •

edited

Loading

UPstartDeveloper Jul 25, 2020

UPstartDeveloper Jul 25, 2020 •

edited

Loading

UPstartDeveloper Jul 25, 2020

uyennguyen16900 Jul 25, 2020

UPstartDeveloper Jul 25, 2020 •

edited

Loading

UPstartDeveloper Jul 25, 2020

UPstartDeveloper Jul 25, 2020

UPstartDeveloper Jul 25, 2020

uyennguyen16900 Jul 25, 2020

UPstartDeveloper left a comment

uyennguyen16900 commented Jul 25, 2020

UPstartDeveloper commented Jul 25, 2020

Un dev #63

Are you sure you want to change the base?

Un dev #63

Conversation

uyennguyen16900 commented Jul 25, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

uyennguyen16900 Jul 25, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

UPstartDeveloper Jul 25, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

UPstartDeveloper Jul 25, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

UPstartDeveloper left a comment

Choose a reason for hiding this comment

uyennguyen16900 commented Jul 25, 2020

UPstartDeveloper commented Jul 25, 2020

uyennguyen16900 commented Jul 25, 2020 •

edited

Loading

uyennguyen16900 Jul 25, 2020 •

edited

Loading

UPstartDeveloper Jul 25, 2020 •

edited

Loading

UPstartDeveloper Jul 25, 2020 •

edited

Loading