test: Added test for created_at #6728

me-diru · 2020-01-07T17:37:35Z

Fixes #6722

Short description of what this resolves:

Checks that created_at is set to current time in all models

Changes proposed in this pull request:

Added test for created_at if set to the current time.

Checklist

I have read the Contribution & Best practices Guide and my PR follows them.
My branch is up-to-date with the Upstream development branch.
The unit tests pass locally with my changes
I have added tests that prove my fix is effective or that my feature works
I have added necessary documentation (if appropriate)

All the functions created/modified in this PR contain relevant docstrings.

tests/all/unit/api/validation/test_createdat.py

houndci-bot · 2020-01-07T17:37:42Z

tests/all/unit/api/validation/test_createdat.py

+        #all the models having created_at are stored in a set 
+        created_at_models=[Session, User, Event , AccessCode, Order, Session, TicketHolder, RoleInvite, DiscountCode, UserTokenBlackListTime, EventInvoice]
+        for model in created_at_models:
+            #looping through each model in a subtest to check if created_at time is set to current time


block comment should start with '# '

tests/all/unit/api/validation/test_createdat.py

houndci-bot · 2020-01-07T17:37:43Z

tests/all/unit/api/validation/test_createdat.py

+from app.models.user_token_blacklist import UserTokenBlackListTime
+from app.models.event_invoice import EventInvoice
+
+class TestCreatedatValidation(TestCase):


expected 2 blank lines, found 1

houndci-bot · 2020-01-07T17:37:43Z

tests/all/unit/api/validation/test_createdat.py

+from app.api import routes
+
+from unittest import TestCase
+#importing all models containing createdat variable


block comment should start with '# '

tests/all/unit/api/validation/test_createdat.py

houndci-bot · 2020-01-07T17:37:44Z

tests/all/unit/api/validation/test_createdat.py

@@ -0,0 +1,35 @@
+import unittest
+#improting datetime


Black would make changes.
block comment should start with '# '

tests/all/unit/api/validation/test_createdat.py

houndci-bot · 2020-01-07T18:50:43Z

tests/all/unit/api/validation/test_createdat.py

+        for model in created_at_models:
+            #looping through each model in a subtest to check if created_at time is set to current time
+            with self.subTest(model=model):
+                self.assertEqual(model.created_at,datetime.now(),'\nTests created_at in model {}'.format(model))


missing whitespace after ','

houndci-bot · 2020-01-07T18:50:43Z

tests/all/unit/api/validation/test_createdat.py

+        '''
+        #all the models having created_at are stored in a set 
+        created_at_models=[Session, User, Event , AccessCode, Order, Session, TicketHolder, 
+                            RoleInvite, DiscountCode, UserTokenBlackListTime, EventInvoice]


continuation line over-indented for visual indent

houndci-bot · 2020-01-07T18:50:44Z

tests/all/unit/api/validation/test_createdat.py

+        ''' createdat validate time : Tests created_at variable in all models 
+        '''
+        #all the models having created_at are stored in a set 
+        created_at_models=[Session, User, Event , AccessCode, Order, Session, TicketHolder, 


missing whitespace around operator
trailing whitespace
whitespace before ','

houndci-bot · 2020-01-07T18:50:44Z

tests/all/unit/api/validation/test_createdat.py

+class TestCreatedatValidation(TestCase):
+
+    def test_createdat_all_models(self):
+        ''' createdat validate time : Tests created_at variable in all models 


trailing whitespace

mrsaicharan1 · 2020-01-08T04:02:52Z

tests/all/unit/api/validation/test_createdat.py

+        for model in created_at_models:
+            #looping through each model in a subtest to check if created_at time is set to current time
+            with self.subTest(model=model):
+                self.assertEqual(model.created_at,datetime.now(),'\nTests created_at in model {}'.format(model))


Are you trying to test for the default attribute of created_at in each model here? If so,
you'll need to instantiate the Alchemy object and then assert its attribute.

I am testing already instantiated attribute of created_at in each model to check if it is set to the current timezone. Yeah, instantiating objects and asserting its attribute can also be done if preferred.

It's not about preference. This'll never work as you can see in the build failure. It's like saying Session.created_at == date. Session is a class, not an object/instance of session so it has no created_at attribute at all

Recommend running tests locally to verify that they pass and what you're doing is correct before sending in PR

Do we need to retrieve the value from db and check or if there is any other way.
After creating an object is there any method returning the object with loaded attributes.
As direct instantiation of object created_at value is stored as None type, as the default is func.now() in class definition of model classes.

Do we need to retrieve the value from db and check

Yes

me-diru · 2020-01-08T20:17:58Z

@iamareebjamal
Can you please look into this in your free time?
Thank you

iamareebjamal · 2020-01-08T20:27:57Z

@mrsaicharan1 Already pointed out the issue. And as you can see, the build is failing. Try to run the tests locally to first see that they are actually passing. Will be happy to review once they do

iamareebjamal · 2020-01-08T20:28:54Z

Also, all hound violations should be fixed as well

me-diru · 2020-01-23T10:42:36Z

I retrieved the value from DB successfully, but there's a small time lag in between while checking with datetime.now()
Can I please get the date format used and is millisecond difference ok while comparing.

iamareebjamal · 2020-01-23T10:54:26Z

Yes, ms difference is OK

mrsaicharan1 · 2020-01-23T11:45:32Z

tests/all/unit/api/validation/test_createdat.py

+        for model in created_at_models:
+            #looping through each model in a subtest to check if created_at time is set to current time
+            with self.subTest(model=model):
+                self.assertEqual(model.created_at,datetime.now(),'\nTests created_at in model {}'.format(model))


Because of the millisecond difference, try testing if the difference between the two is not greater than certain time difference (maybe 50ms or 100ms)

me-diru · 2020-01-23T18:11:34Z

Not all models created_at have a default value set to test, should I check models which have it set as I am getting None value if not specified?

iamareebjamal · 2020-01-23T18:50:27Z

If you're getting None, that means you've caught an issue with the model. Fix the model so it returns date

me-diru · 2020-01-24T05:00:14Z

Some models have a fixedOffset datetime which shouldn't be the case I suppose as all datetime aware objects are set by default to UTC standard

iamareebjamal · 2020-01-24T07:02:03Z

You should not test greater than or equal to. That's wrong. You should test that difference between now and stored date is less than a threshold value

me-diru · 2020-01-26T19:38:06Z

You should not test greater than or equal to. That's wrong. You should test that difference between now and stored date is less than a threshold value

The datetime data retrieved from db is coming with some offset which is not compatible with UTC standards. There is no datetime function to get db version of datetime i.e +5:30 value as I got.

iamareebjamal · 2020-01-26T20:22:18Z

There is no datetime function to get db version of datetime i.e +5:30 value as I got.

I cannot understand what you mean by that

The datetime data retrieved from db is coming with some offset which is not compatible with UTC standards.

Which UTC standard does it not follow? Can you show me the clause in the standard which it violates?

me-diru · 2020-01-26T21:10:27Z

The datetime data retrieved from db is coming with some offset which is not compatible with UTC standards.

Which UTC standard does it not follow? Can you show me the clause in the standard which it violates?

It didn't violate any standard perse, there was a difference of 5:30 gap between the both as you can see. Fixed it by using "datetime.utcnow().astimezone()" for getting time with local offset in accordance with the db.

tests/all/unit/api/validation/test_createdat.py

codecov · 2020-01-26T21:39:30Z

Codecov Report

Merging #6728 into development will decrease coverage by <.01%.
The diff coverage is n/a.

@@               Coverage Diff               @@
##           development    #6728      +/-   ##
===============================================
- Coverage         65.5%   65.49%   -0.01%     
===============================================
  Files              306      306              
  Lines            15346    15346              
===============================================
- Hits             10052    10051       -1     
- Misses            5294     5295       +1

Impacted Files	Coverage Δ
app/api/schema/events.py	`91.47% <ø> (ø)`	⬆️
app/models/event.py	`78.09% <0%> (-0.36%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 91e0de8...dfd2b3e. Read the comment docs.

tests/all/unit/api/validation/test_createdat.py

app/models/role_invite.py

app/models/discount_code.py

houndci-bot · 2020-01-27T21:00:30Z

tests/all/unit/api/validation/test_createdat.py

+            time_diff = current_time - db_created_at
+            allowed_time_lag = timedelta(milliseconds = 50)
+            self.assertLessEqual(time_diff, allowed_time_lag, 
+                "EventInvoice model created_at not set to current time")


continuation line under-indented for visual indent

houndci-bot · 2020-01-27T21:00:30Z

tests/all/unit/api/validation/test_createdat.py

+            current_time = datetime.utcnow().astimezone()
+            time_diff = current_time - db_created_at
+            allowed_time_lag = timedelta(milliseconds = 50)
+            self.assertLessEqual(time_diff, allowed_time_lag, 


trailing whitespace

houndci-bot · 2020-01-27T21:00:30Z

tests/all/unit/api/validation/test_createdat.py

+            db_created_at = db.session.query(EventInvoice).get(event_invoice_id).created_at
+            current_time = datetime.utcnow().astimezone()
+            time_diff = current_time - db_created_at
+            allowed_time_lag = timedelta(milliseconds = 50)


unexpected spaces around keyword / parameter equals

houndci-bot · 2020-01-27T21:00:30Z

tests/all/unit/api/validation/test_createdat.py

+            test_event_invoice = EventInvoice()
+            save_to_db(test_event_invoice)
+            event_invoice_id = test_event_invoice.id
+            db_created_at = db.session.query(EventInvoice).get(event_invoice_id).created_at


line too long (91 > 90 characters)

tests/all/unit/api/validation/test_createdat.py

me-diru · 2020-01-27T21:09:49Z

Hello @iamareebjamal ,
I have successfully made proper working tests for each model. Can you please go through them and give your thoughts on it.

iamareebjamal · 2020-01-27T22:26:42Z

I think this PR just broke the record of violations/lines changed

iamareebjamal · 2020-01-27T22:29:51Z

Your PR has 86 file changes. Rebase with development and push. I can only review when opening this page https://github.com/fossasia/open-event-server/pull/6728/files does not require 400% of my CPU. I don't want to brick my laptop

tests/all/unit/api/validation/test_createdat.py

iamareebjamal · 2020-02-01T20:07:55Z

Why haven't you run pre-commit install?

iamareebjamal · 2020-02-01T20:37:38Z

There's no use of marking issues resolved. I can still see them and you will still need to resolve them

me-diru · 2020-02-01T20:43:31Z

Sir, actually no changes are coming when I run black, and also I have run pre-commit as you said and it didn't show any changes.

iamareebjamal · 2020-02-01T20:49:11Z

app/factories/access_code.py

@@ -11,6 +11,7 @@ class AccessCodeFactory(factory.alchemy.SQLAlchemyModelFactory):
    class Meta:
        model = AccessCode
        sqlalchemy_session = db.session
+        sqlalchemy_session_persistence = 'commit'


Why add this?

sir, as created_at can't be assigned any value in a factory. we need to commit it in db to retrieve actually stored value in db so that is used to commit.

Then commit manually. No need of this

iamareebjamal · 2020-02-01T20:49:30Z

app/factories/access_code.py

    valid_from = common.date_
    valid_till = common.dateEnd_
-    event_id = 1


Revert these changes

can revert in access code without any error. But if I do the same in Session factory the following error occurs.

See how it is done in hook files

iamareebjamal · 2020-02-01T20:50:01Z

app/factories/ticket_holder.py

+from app.models.ticket_holder import TicketHolder, db
+
+
+class TicketHolderFactory(factory.alchemy.SQLAlchemyModelFactory):


AttendeeFactory already exists

will remove thanks!

me-diru · 2020-02-02T06:59:00Z

test_export_sessions_with_details_csv and test_delete_ticket_holders_with_no_order_id are failing in travis build.
The first is depending on created_at = common.date_ in SessionFactory and getting some import error on the other when checked locally.
Can you please take a look into this

iamareebjamal · 2020-02-02T07:14:51Z

Since you have removed created_at from factories, you have to fix both tests to restore old behaviour

me-diru · 2020-02-02T09:37:38Z

Able to solve test_csv_jobs_util error.
I am not able to run test_scheduled_jobs locally.
What I have seen from the code so far is :

the function which deletes the ticket holder with no order_id, I don't know why it is using deleted_at(which isn't present )and created_at from TicketHolder in querying.

iamareebjamal · 2020-02-02T11:26:02Z

You have to fix the test, not the schedule task

me-diru · 2020-02-02T11:39:18Z

actually, the test is dependent on the schedule task. Before it somehow deleted the ticketHolder and test passed. Now it is not doing so

iamareebjamal · 2020-02-02T11:43:31Z

schedule task is dependent on the test. First the created_at was set at 2016, now it is set to current time. How is it going to work?

Before it somehow deleted the ticketHolder and test passed. Now it is not doing so

There is no somehow with computers. There's always a reason. They are deterministic machines

me-diru · 2020-02-02T11:54:49Z

Sir, I just overthought it before as expiry time is added to it. I thought If it checks with 2016, there's no point in checking it. Now I understand the use of tests a bit more.

Please review my submission, made all changes as mentioned.

updated with upstream

iamareebjamal · 2020-02-02T12:40:46Z

tests/all/unit/api/validation/test_createdat.py

+                    if model_factory in models_with_utcnow:
+                        current_time = datetime.utcnow().astimezone()
+                    else:
+                        current_time = datetime.now(timezone.utc).astimezone()


There shouldn't be any difference. Please make them consistent

There's actually a difference, as you can see in the below image.
Printing in this order:

Output:

I never said there isn't a difference. I said there shouldn't be a difference

The values am getting are weird, one has date with an offset added and specifying the offset[1]. Other one has no offset set also coming with offset added[2].
There is no python function to standardize these two.

2020-02-02 20:24:32.051071+05:30

2020-02-02 14:54:32.064461+05:30

iamareebjamal · 2020-02-02T15:16:06Z

Thanks for contributing and being patient :)

niranjan94 · 2020-02-02T15:21:09Z

Here is an overview of what got changed by this pull request:

Complexity increasing per file
==============================
- app/factories/user_token_blacklist.py  1
- tests/all/unit/api/validation/test_createdat.py  5

See the complete overview on Codacy

houndci-bot reviewed Jan 7, 2020

View reviewed changes

mrsaicharan1 reviewed Jan 8, 2020

View reviewed changes

mrsaicharan1 reviewed Jan 23, 2020

View reviewed changes

houndci-bot reviewed Jan 26, 2020

View reviewed changes

houndci-bot reviewed Jan 27, 2020

View reviewed changes

houndci-bot reviewed Jan 28, 2020

View reviewed changes

me-diru added 2 commits February 2, 2020 01:56

used factories for testing and added new ones

95832a0

did pre-commit

9944d37

iamareebjamal reviewed Feb 1, 2020

View reviewed changes

me-diru added 2 commits February 2, 2020 03:08

made changes as requested

0735ccf

changed user_blacklist factory to correct one

f6fba0e

correct test_csv_jobs_util for travis

cb91457

me-diru added 2 commits February 2, 2020 16:38

updated branch in travis for local testing

c55c679

cahnged test_schedule_jobs

c6f6fa9

corrected test_scheduled_jobs

75aa289

Merge pull request #7 from fossasia/development

91e0de8

updated with upstream

iamareebjamal changed the title ~~Added test for createdat~~ test: Added test for created_at Feb 2, 2020

auto-label bot added the testing label Feb 2, 2020

iamareebjamal reviewed Feb 2, 2020

View reviewed changes

iamareebjamal approved these changes Feb 2, 2020

View reviewed changes

Merge branch 'development' into development

dfd2b3e

iamareebjamal merged commit 2afce3b into fossasia:development Feb 2, 2020

		from app.models.ticket_holder import TicketHolder, db


		class TicketHolderFactory(factory.alchemy.SQLAlchemyModelFactory):

test: Added test for created_at #6728

test: Added test for created_at #6728

Conversation

me-diru commented Jan 7, 2020 • edited Loading

Short description of what this resolves:

Changes proposed in this pull request:

Checklist

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

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

iamareebjamal Jan 8, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

me-diru commented Jan 8, 2020

iamareebjamal commented Jan 8, 2020

iamareebjamal commented Jan 8, 2020

me-diru commented Jan 23, 2020

iamareebjamal commented Jan 23, 2020

Choose a reason for hiding this comment

me-diru commented Jan 23, 2020

iamareebjamal commented Jan 23, 2020

me-diru commented Jan 24, 2020 • edited Loading

iamareebjamal commented Jan 24, 2020

me-diru commented Jan 26, 2020

iamareebjamal commented Jan 26, 2020 • edited Loading

me-diru commented Jan 26, 2020

codecov bot commented Jan 26, 2020 • edited Loading

Codecov Report

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

me-diru commented Jan 27, 2020

iamareebjamal commented Jan 27, 2020

iamareebjamal commented Jan 27, 2020

iamareebjamal commented Feb 1, 2020

iamareebjamal commented Feb 1, 2020

me-diru commented Feb 1, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

me-diru commented Feb 2, 2020 • edited Loading

iamareebjamal commented Feb 2, 2020

me-diru commented Feb 2, 2020 • edited Loading

iamareebjamal commented Feb 2, 2020

me-diru commented Feb 2, 2020

iamareebjamal commented Feb 2, 2020

me-diru commented Feb 2, 2020

Choose a reason for hiding this comment

me-diru Feb 2, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iamareebjamal commented Feb 2, 2020

niranjan94 commented Feb 2, 2020

me-diru commented Jan 7, 2020 •

edited

Loading

iamareebjamal Jan 8, 2020 •

edited

Loading

me-diru commented Jan 24, 2020 •

edited

Loading

iamareebjamal commented Jan 26, 2020 •

edited

Loading

codecov bot commented Jan 26, 2020 •

edited

Loading

me-diru commented Feb 1, 2020 •

edited

Loading

me-diru commented Feb 2, 2020 •

edited

Loading

me-diru commented Feb 2, 2020 •

edited

Loading

me-diru Feb 2, 2020 •

edited

Loading