Added admin FFQ code lookup #555
Conversation
| "RETURNING interested_user_id" | ||
| ) | ||
| iu_ids = [tup[0] for tup in cur.fetchall()] | ||
| t.commit() |
There was a problem hiding this comment.
Can these state changes be rolled back either in tearDown or can the test be fully executed w/o commiting the transaction?
There was a problem hiding this comment.
I was hesitant to do so because:
- These operations are naive and does not conform to normal workflow such as when an ftp/ffq code will be created but directly inserting information related to this query. Running other operations will probably trigger lots of logic problems;
- Some tests were in-between the insertions for convenience, but I could reformat them to not be affected by one insertion;
- I tried to contain all insertion and deletion in the same test function so that running that function won't affect other workflows
There was a problem hiding this comment.
It's standard practice in this codebase, and others under the biocore organization, to not leave lingering state changes in databases during testing. Short term, it might simplify a test. Long term, it makes reasoning about what's going on difficult as the state of the entity being tested is changing. It can also make repeat runs of the same tests, without repopulating the test database in between, impossible
There are a few different ways it's managed in this codebase depending on the situation, see e.g., repo/source.py. I'm not sure what strategy will be best here. It might be to use exact identifiers on population, and have tearDown remove from the corresponding tables which is practical but leaves room for missing a few things and possibly some maintenance burden. Best is if this can be done where a .rollback() can just be issued, but that is not possible in all scenarios
There was a problem hiding this comment.
Thanks for the information! Yes I agree that we should try to place all database state changes in setUp and tearDown, so is it okay if the inserted values have logic errors? For example, if a side effect of inserting a ftp involves populating another table and this never happens. We could make sure to only use these values for the purpose of this query.
There was a problem hiding this comment.
Yes, exactly. If state is always reset, then it's fine to put garbage in to verify behavior.
Test execution is out-of-order, so any state changes which occur can have side effects on other tests. This can lead to difficult to debug/understand scenarios when bad data are intentionally placed into the database.
| for tx_id, iu_id in tx_ids.items() | ||
| ]) | ||
| ) | ||
| t.commit() |
| tup[0]: tx_id | ||
| for tup, tx_id in zip(cur.fetchall(), tx_ids) | ||
| } | ||
| t.commit() |
| email = "%" + email + "%" | ||
| with self._transaction.dict_cursor() as cur: | ||
| # Note: Use left join to differentiate email not found possibility | ||
| cur.execute( |
There was a problem hiding this comment.
+1, impressive query
|
Going to hold on merging this until the corresponding Interface PR is finalized. Great work @colts661 ! |
No description provided.