Integration Test Case for is_latest_issue using DB #925

xavier-xia-99 · 2022-06-06T23:50:49Z

Summary

This is a fix for issue #896.

The earlier integration tests did not consist of a connection to the database, instead using a MagicMock() object. This did not emulate the actual behaviour of the system. This PR includes a test file is_latest_issue.py where the database was actively used to query to ensure that the is_latest_issue flag is accurately reflected in signal_latest and signal_history. It also checks for the addition of new src , sig and geo_value, geo_type into respective *_dim tables.

It also includes an edge case that was identified with regards to the db pipeline.

Fixes:

syntax error failing CI
fixes [Refactor] insert_or_update_bulk is a thin wrapper around insert_or_update_batch without a purpose? #926

added edge case added prettytable for better vizuals all tests passed

melange396

you should revert test_covidcast_meta_caching.py (its changes are minuscule and not part of this issue) and delete dev/local/output.txt

there are a lot of repeated strings in the SQL statements. they can be saved to variables for re-use -- which can also mean less reading for anyone doing maintenance on this.

i think i would remove the print() statements and prettytable and maxDiff=None stuff, unless you think theyre particularly useful. or maybe just reduce them to one call to your view_table() helper fn in the test_*() methods?

clean up these and the other comments, and we can take another pass at it!

integrations/acquisition/covidcast/test_is_latest_issue.py

requirements.txt

integrations/acquisition/covidcast/test_is_latest_issue.py

melange396 · 2022-06-07T17:31:06Z

integrations/acquisition/covidcast/test_is_latest_issue.py

+    sql2 = '''SELECT `issue` FROM `signal_history` where `time_value` '''
+    self._db._cursor.execute(sql2)
+    record3 = self._db._cursor.fetchall()
+    self.assertEqual(3,self.totalRows + 1) #ensure 3 added (1 of which refreshed)


i dont think this is checking what you want... totalRows doesnt get updated and is still equal to 2, which you set at the very top of this test

Here was what I was trying to test: Initially, there are 2 rows and I update one of them and added a new one, so I wanted to make sure that there were only 3 rows in the end. If this is confusing, I am trying to think of better ways to test it.
(1 old & not updated)
(1 old but updated)
(1 added newly)

melange396 · 2022-06-07T17:32:59Z

integrations/acquisition/covidcast/test_is_latest_issue.py

+    self.assertEqual(20200416,max(list(record3))[0]) #max of the outputs is 20200416 , extracting from tuple
+
+    #check older issue not inside latest, empty field
+    sql = '''SELECT * FROM `signal_latest` where `time_value` = 20200414 and `issue` = 20200415 '''


did you mean issue=14? theres never an issue=15 with time_value=14

Thank you for that! Removed issue = 15 totally too from the dummy data

melange396 · 2022-06-07T17:34:03Z

integrations/acquisition/covidcast/test_is_latest_issue.py

+    #setting baseline variables
+    self._db._cursor.execute('''SELECT * FROM `geo_dim`''')
+    record = self._db._cursor.fetchall()
+    self.geoDimRows = len(list(record))


like before, these variables dont need to be attached to the self object

melange396 · 2022-06-07T17:48:06Z

integrations/acquisition/covidcast/test_is_latest_issue.py

+      oldSrcSig = [
+        CovidcastRow('src', 'sig', 'day', 'state', 20211111, 'pa', #new src, new sig
+                    99, 99, 99, nmv, nmv, nmv, 20211111, 1),
+        CovidcastRow('src', 'sig', 'day', 'county', 20211111, 'ca', #new src, new sig


maybe use a geo_value that doesnt look like a state abbreviation when you have a geo_type of "county" (just for reader clarity, it doesnt actually matter for the test)

That's a good point! What are some sample geo_value normally used?

Any length-5 string of numbers. "11111" should suffice

Co-authored-by: melange396 <[email protected]>

removed additional BASE_URL Co-authored-by: melange396 <[email protected]>

remove unnecessary package Co-authored-by: melange396 <[email protected]>

…entry"

melange396

Looks pretty good, just a few little things:

there are still some unused imports (including prettytable, so you can also remove that from requirements.txt)
you should revert the changes in integrations/acquisition/covidcast/test_covidcast_meta_caching.py
check your comments on the CovidcastRow() lines, i think some need to be updated
some spacing nits (usually want an empty line right before a class or method definition line, usually dont want an empty line right after; probably dont want double empty lines unless separating important or otherwise large sections of code)

integrations/acquisition/covidcast/test_is_latest_issue.py

melange396 · 2022-06-14T13:48:51Z

integrations/acquisition/covidcast/test_is_latest_issue.py

+    self._db.insert_or_update_bulk(rows)
+    self._db.run_dbjobs()
+    #preview
+    self._db._cursor.execute('''SELECT * FROM `signal_history`''')


use self.viewSignalHistory?

melange396 · 2022-06-14T13:59:19Z

integrations/acquisition/covidcast/test_is_latest_issue.py

+    #dynamic check for signal_history
+    self._db._cursor.execute('''SELECT `issue` FROM `signal_history`''')
+    record3 = self._db._cursor.fetchall()
+    self.assertEqual(2,totalRows + 1) #ensure 3 added (1 of which refreshed)


would it make more sense to check len(record3) here?

melange396 · 2022-06-14T16:15:52Z

integrations/acquisition/covidcast/test_is_latest_issue.py

+    rows = [
+      CovidcastRow('src', 'sig', 'day', 'state', 20200414, 'pa', # 
+      2, 2, 2, nmv, nmv, nmv, 20200414, 0),
+      CovidcastRow('src', 'sig', 'day', 'county', 20200414, '11111', # updating previous entry


thats not an update!

melange396 · 2022-06-14T16:16:28Z

integrations/acquisition/covidcast/test_is_latest_issue.py

+  def test_src_sig(self):
+    #BASE CASES
+    rows = [
+      CovidcastRow('src', 'sig', 'day', 'state', 20200414, 'pa', # 


Suggested change

CovidcastRow('src', 'sig', 'day', 'state', 20200414, 'pa', #

CovidcastRow('src', 'sig', 'day', 'state', 20200414, 'pa',

krivard · 2022-06-14T19:50:25Z

integrations/acquisition/covidcast/test_is_latest_issue.py

+    self.viewSignalLatest = '''SELECT * FROM `signal_latest`'''
+    self.viewSignalHistory = '''SELECT * FROM `signal_history`'''
+    self.viewSignalDim = '''SELECT `source`, `signal` FROM `signal_dim`'''
+    self.viewGeoDim = '''SELECT `geo_type`,`geo_value` FROM `geo_dim`'''


For easier maintainability, use e.g. Database.history_table instead of "signal_history" in these

Co-authored-by: melange396 <[email protected]>

melange396 · 2022-06-16T18:49:06Z

integrations/acquisition/covidcast/test_is_latest_issue.py

+"""Integration tests for covidcast's is_latest_issue boolean."""
+# standard library
+import unittest
+import time


Suggested change

import time

melange396 · 2022-06-16T18:49:20Z

integrations/acquisition/covidcast/test_is_latest_issue.py

+# standard library
+import unittest
+import time
+import threading


Suggested change

import threading

melange396 · 2022-06-16T18:49:36Z

integrations/acquisition/covidcast/test_is_latest_issue.py

+
+
+# third party
+from aiohttp.client_exceptions import ClientResponseError


Suggested change

from aiohttp.client_exceptions import ClientResponseError

melange396 · 2022-06-16T18:49:54Z

integrations/acquisition/covidcast/test_is_latest_issue.py

+import mysql.connector
+import pytest
+# first party
+from delphi.epidata.acquisition.covidcast.logger import get_structured_logger


Suggested change

from delphi.epidata.acquisition.covidcast.logger import get_structured_logger

melange396 · 2022-06-16T18:50:15Z

integrations/acquisition/covidcast/test_is_latest_issue.py

+from delphi_utils import Nans
+from delphi.epidata.client.delphi_epidata import Epidata
+from delphi.epidata.acquisition.covidcast.database import Database, CovidcastRow
+from delphi.epidata.acquisition.covidcast.covidcast_meta_cache_updater import main as update_covidcast_meta_cache


Suggested change

from delphi.epidata.acquisition.covidcast.covidcast_meta_cache_updater import main as update_covidcast_meta_cache

melange396 · 2022-06-16T18:50:46Z

integrations/acquisition/covidcast/test_is_latest_issue.py

+# third party
+from aiohttp.client_exceptions import ClientResponseError
+import mysql.connector
+import pytest


Suggested change

import pytest

krivard

Changes for maintainability and a question

krivard · 2022-06-24T17:22:03Z

integrations/acquisition/covidcast/test_is_latest_issue.py

+    #dynamic check for signal_history's list of issue
+    self._db._cursor.execute(f'SELECT `issue` FROM {Database.history_table}')
+    record3 = self._db._cursor.fetchall()
+    self.assertEqual(len(record3),totalRows + 1) #ensure 3 added (1 of which refreshed)


If len(record3) is equal to totalRows + 1 but totalRows + 1 is not equal to 3, this will pass. Is that intended?

integrations/acquisition/covidcast/test_is_latest_issue.py

krivard · 2022-07-11T18:38:54Z

integrations/acquisition/covidcast/test_is_latest_issue.py

+      #ensure new entries are added in latest
+      self._db._cursor.execute(self.viewSignalLatest)
+      record = self._db._cursor.fetchall()
+      self.assertEqual(len(list(record)), sigLatestRows + 2) #2 original, 2 added


A pattern like this will be easier to maintain -- we'll be able to add new tests without having to update all subsequent expressions.

Suggested change

self.assertEqual(len(list(record)), sigLatestRows + 2) #2 original, 2 added

record_length = len(list(record))

self.assertEqual(record_length, sigLatestRows + 2) #2 original, 2 added

sigLatestRows = record_length

krivard · 2022-07-11T18:39:50Z

integrations/acquisition/covidcast/test_is_latest_issue.py

+      self._db._cursor.execute(self.viewSignalDim)
+      record = self._db._cursor.fetchall()
+      res = [('src', 'sig'), ('new_src', 'sig'), ('src', 'new_sig')]
+      self.assertEqual(res , (record))


Update sigDimRows here

krivard · 2022-07-11T18:40:09Z

integrations/acquisition/covidcast/test_is_latest_issue.py

+
+      self._db._cursor.execute(self.viewSignalLatest)
+      record = self._db._cursor.fetchall()
+      self.assertEqual(len(list(record)),sigLatestRows + 6) #total entries = 2(initial) + 6(test)


Update sigLatestRows here

krivard · 2022-07-11T18:40:23Z

integrations/acquisition/covidcast/test_is_latest_issue.py

+
+      self._db._cursor.execute(f'SELECT `geo_type`,`geo_value` FROM `geo_dim`')
+      record = self._db._cursor.fetchall()
+      self.assertEqual(len(list(record)),geoDimRows + 3) #2 + 3 new


Update geoDimRows here

krivard · 2022-07-11T18:40:44Z

integrations/acquisition/covidcast/test_is_latest_issue.py

+
+      self._db._cursor.execute(self.viewSignalLatest)
+      record = self._db._cursor.fetchall()
+      self.assertEqual(len(list(record)),sigLatestRows + 6 + 3) #total entries = 2(initial) + 6(test)  


Update sigLatestRows here

krivard · 2022-07-11T18:56:27Z

integrations/acquisition/covidcast/test_is_latest_issue.py

+    self._db.run_dbjobs()
+    self._db._cursor.execute(f'SELECT `issue` FROM {Database.latest_table} ')
+    record = self._db._cursor.fetchall()  
+    self.assertEqual(record[0][0], 20200417) #20200416 != 20200417


Suggested change

self.assertEqual(record[0][0], 20200417) #20200416 != 20200417

# Make sure the 4/17 issue is listed even though 4/16 was imported after it

self.assertEqual(record[0][0], 20200417)

krivard · 2022-07-26T15:31:20Z

integrations/acquisition/covidcast/test_dim_tables.py

+<<<<<<< HEAD
            res = [('src', 'sig'), ('new_src', 'sig'), ('src', 'new_sig')]
            self.assertEqual(res , (record))
+=======
+            self.sigDimRows = len(list(record))
+
+            res = set([('new_src', 'sig'), ('src', 'new_sig'), ('src', 'sig')])
+            self.assertEqual(res , set(record))
+            self.assertEqual(3, self.sigDimRows)
+>>>>>>> 8a39f36b (used set to remove ordering of elements in return list)


Looks like the conflict edit didn't stick -- are you okay to fix?

krivard

Needs a thorough pass through to:

change object members to local variables (except where they reduce duplication, like in self.viewSignalLatest)
either strip out redundant line comments or add context that explains the reasoning behind the operation.

krivard · 2022-08-08T21:08:10Z

integrations/acquisition/covidcast/test_dim_tables.py

        self._db.run_dbjobs()
        self._db._cursor.execute(self.viewGeoDim)
        record = self._db._cursor.fetchall()
-        geoDimRows = len(list(record))
+        self.geoDimRows = len(list(record))


Does geoDimRows need to be an object member, or can it be a local variable instead? Remember, test methods probbly shouldnt alter the TestCase object without a good reason

krivard · 2022-08-08T21:15:22Z

integrations/acquisition/covidcast/test_is_latest_issue.py


    #sanity check for adding dummy data
    sql = f'SELECT `issue` FROM {Database.latest_table} where `time_value` = 20200414'
    self._db._cursor.execute(sql)
    record = self._db._cursor.fetchall()
    self.assertEqual(record[0][0], 20200414)
-    self.assertEqual(len(record), 1) #check 1 entry
+    self.assertEqual(len(record), 1) #check 1 entry present


line comments should add context for future readers, beyond just restating the code -- for example, this modification explains why we only expect 1 entry for this query:

Suggested change

self.assertEqual(len(record), 1) #check 1 entry present

self.assertEqual(len(record), 1) #placeholder data only has one issue for 20200414

I edited them accordingly, let me know if they showed up!

xavier-xia-99 added 2 commits June 6, 2022 14:31

added integrations test for database for is_latest_issue

bd7f285

added edge case added prettytable for better vizuals all tests passed

syntax error

d789236

xavier-xia-99 requested review from melange396 and krivard June 6, 2022 23:50

xavier-xia-99 changed the title ~~Integration Test Case for Database~~ Integration Test Case for is_latest_issue using DB Jun 6, 2022

melange396 requested changes Jun 7, 2022

View reviewed changes

xavier-xia-99 and others added 12 commits June 8, 2022 21:34

Update integrations/acquisition/covidcast/test_is_latest_issue.py

c3f0205

Co-authored-by: melange396 <[email protected]>

Update integrations/acquisition/covidcast/test_is_latest_issue.py

1f10a04

removed additional BASE_URL Co-authored-by: melange396 <[email protected]>

Update requirements.txt

b241980

remove unnecessary package Co-authored-by: melange396 <[email protected]>

added line to ignore output.txt

0a4e09e

tidied up code and provided common SQL queries as variables

f0f3abe

removed unnecessary imports

a998d38

added line to ignore output.txt

035baf7

removed unnecessary row for signal_latest, checked there is only one …

6d8d25e

…entry"

removed unnecessary self.

4090d4d

changed (county,ca) to (county,numeric) pair

2e0546f

Delete output.txt

39ab346

showTable function WIP

db706ac

melange396 requested changes Jun 14, 2022

View reviewed changes

krivard reviewed Jun 14, 2022

View reviewed changes

Base automatically changed from v4-schema-revisions-release-prep-prep to v4-schema-revisions-release-prep June 15, 2022 20:14

xavier-xia-99 and others added 5 commits June 16, 2022 04:24

added Epidata for BASE_URL

b9d712f

Co-authored-by: melange396 <[email protected]>

tidied up comments and test cases

d0b8d69

changed all SQL queries to f-strings as better practice

11a5316

reverted test_covidcast_meta_caching

64ea75d

all done;

9cd42c3

melange396 requested changes Jun 16, 2022

View reviewed changes

tidied up imports

00e396b

krivard suggested changes Jul 11, 2022

View reviewed changes

removed dim tests

dee268d

xavier-xia-99 added 2 commits July 12, 2022 12:30

added dim tests in new file

157530f

used set to remove ordering of elements in return list

18a2758

xavier-xia-99 force-pushed the xavier/integration/tests branch from 8a39f36 to 18a2758 Compare July 26, 2022 15:22

krivard reviewed Jul 26, 2022

View reviewed changes

xavier-xia-99 added 4 commits July 26, 2022 10:37

used set to remove ordering of elements in return list

cb78c73

edited dim_tables for better readability

6682b58

syntax and functionality checked

f06c327

file edits

d250f98

krivard suggested changes Aug 8, 2022

View reviewed changes

xavier-xia-99 added 4 commits August 9, 2022 09:55

removed unnecessary test objects

56852f4

refined line comments

7427906

refined comments and added tests for geo_type, geo_value pairs

c120316

clean up redundant comments

1f6322c

xavier-xia-99 requested review from krivard and melange396 August 31, 2022 18:06

Base automatically changed from v4-schema-revisions-release-prep to dev September 22, 2022 14:05

melange396 mentioned this pull request Feb 3, 2023

Write integration tests for src/acquisition/covidcast/database.py #896

Open

2 tasks

melange396 mentioned this pull request May 8, 2024

insert_or_update can have problems with multiple "issue"s #1427

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Integration Test Case for is_latest_issue using DB #925

Integration Test Case for is_latest_issue using DB #925

xavier-xia-99 commented Jun 6, 2022 •

edited by dshemetov

Loading

melange396 left a comment

melange396 Jun 7, 2022

xavier-xia-99 Jun 11, 2022

melange396 Jun 7, 2022

xavier-xia-99 Jun 11, 2022

melange396 Jun 7, 2022

melange396 Jun 7, 2022

xavier-xia-99 Jun 8, 2022

krivard Jun 9, 2022

melange396 left a comment

melange396 Jun 14, 2022

melange396 Jun 14, 2022

melange396 Jun 14, 2022

melange396 Jun 14, 2022

krivard Jun 14, 2022

melange396 Jun 16, 2022

melange396 Jun 16, 2022

melange396 Jun 16, 2022

melange396 Jun 16, 2022

melange396 Jun 16, 2022

melange396 Jun 16, 2022

krivard left a comment

krivard Jun 24, 2022

krivard Jul 11, 2022

krivard Jul 11, 2022

krivard Jul 11, 2022

krivard Jul 11, 2022

krivard Jul 11, 2022

krivard Jul 11, 2022

krivard Jul 26, 2022

krivard left a comment •

edited by xavier-xia-99

Loading

krivard Aug 8, 2022

krivard Aug 8, 2022

xavier-xia-99 Aug 31, 2022

	CovidcastRow('src', 'sig', 'day', 'state', 20200414, 'pa', #
	CovidcastRow('src', 'sig', 'day', 'state', 20200414, 'pa',



		# third party
		from aiohttp.client_exceptions import ClientResponseError

-      self.assertEqual(len(list(record)), sigLatestRows + 2) #2 original, 2 added
+      record_length = len(list(record))
+      self.assertEqual(record_length, sigLatestRows + 2) #2 original, 2 added
+      sigLatestRows = record_length

	self.assertEqual(record[0][0], 20200417) #20200416 != 20200417
	# Make sure the 4/17 issue is listed even though 4/16 was imported after it
	self.assertEqual(record[0][0], 20200417)

	self.assertEqual(len(record), 1) #check 1 entry present
	self.assertEqual(len(record), 1) #placeholder data only has one issue for 20200414

Integration Test Case for is_latest_issue using DB #925

Are you sure you want to change the base?

Integration Test Case for is_latest_issue using DB #925

Conversation

xavier-xia-99 commented Jun 6, 2022 • edited by dshemetov Loading

melange396 left a 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

melange396 left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krivard left a 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

krivard left a comment • edited by xavier-xia-99 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

xavier-xia-99 commented Jun 6, 2022 •

edited by dshemetov

Loading

krivard left a comment •

edited by xavier-xia-99

Loading