Opened 7 weeks ago

Closed 5 weeks ago

Last modified 3 weeks ago

#33191 closed task (fixed)

Write database tests for twisted adbapi for GetTor

Reported by: cohosh Owned by: cohosh
Priority: Medium Milestone:
Component: Applications/GetTor Version:
Severity: Normal Keywords:
Cc: traumschule, hiro, gaba, phw, cohosh, meejah Actual Points: 1
Parent ID: Points: .5
Reviewer: hiro Sponsor:

Description (last modified by cohosh)

We discussed moving to a different sqlite3 api, but after talking with @meejah, it's a bad idea to mix and match twisted with other I/O operations.

So, we're opting to stay with twisted, write some tests that work, and see whether we need to make changes to how dbpool is called and used.

Child Tickets

Change History (13)

comment:1 Changed 7 weeks ago by cohosh

Status: assignedneeds_review

I based these changes off of the commits in #33123 because they restructure the database queries.

The first commit removes a bunch of unused imports that I found through running a linter. The second makes the actual switch from twisted.adbapi to sqlite3.

https://dip.torproject.org/cohosh/gettor/compare/ticket%2F33123...database

comment:2 Changed 7 weeks ago by cohosh

Reviewer: hiro

comment:3 Changed 6 weeks ago by hiro

This LGTM! Also cohosh so nice to have those DB operations finally tested. Thanks for this.

comment:4 Changed 6 weeks ago by hiro

Status: needs_reviewmerge_ready

comment:5 Changed 6 weeks ago by cohosh

Thanks! I just realized this needs a bit more work because the returns for the database functions are no longer deferreds. I've merged what I have, but will work on this fix before we deploy it.

comment:6 Changed 6 weeks ago by cohosh

Status: merge_readyneeds_review

Okay this patch will update the callers of the new database functions.

comment:7 Changed 6 weeks ago by cohosh

Cc: meejah added
Status: needs_reviewneeds_revision

Just had a conversation with meejah in irc. It turns out that using a different database library in a program run by the twisted reactor can cause some issues. I'm going to go back to using twisted-adbapi for this and try some things we discussed to get the tests working.

comment:8 Changed 6 weeks ago by cohosh

@meejah

Alright, this is the trouble I was running into before. Here's a very simple database test (doesn't assert any values, just calls one of the database functions):
https://dip.torproject.org/cohosh/gettor/blob/ticket/33191v2/tests/test_db.py

If I checkout out this branch and run it using

$ python3 scripts/create_db -n -c -o -f tests/gettor.db
$ python3 scripts/add_links_to_db -f tests/gettor.db
$ pytest-3 tests/

It hangs indefinitely. Any ideas on what's going wrong here? The test behaves very similarly to how this function is actually called in the code here:

    @defer.inlineCallbacks
    def get_locales(self):
        dbname = self.settings.get("dbname")
        conn = SQLite3(dbname)

        locales = yield conn.get_locales()
        for l in locales:
            self.locales.append(l[0])

And I get the same results if I try using the pytest-twisted library as follows:

@pytest_twisted.inlineCallbacks
    def test_stored_locales(self):
        locales = yield self.conn.get_locales()

comment:9 Changed 5 weeks ago by cohosh

Summary: Move from twisted adbapi to sqlite3 for GetTorWrite database tests for twisted adbapi for GetTor

comment:10 Changed 5 weeks ago by cohosh

Status: needs_revisionneeds_review

Okay here's a merge request that implements working database tests while sticking with the twisted adbapi.

https://dip.torproject.org/cohosh/gettor/merge_requests/9

Note that one commit actually makes changes to how the database code is called in gettor. Specifically, we weren't ever closing the connection to the database in the gettor code. This should fix that bug as well as get our tests running cleanly.

comment:11 Changed 5 weeks ago by cohosh

Description: modified (diff)

comment:12 Changed 5 weeks ago by cohosh

Resolution: fixed
Status: needs_reviewclosed

Merged at 3dc32 and deployed as of 2020-02-21T18:39:13.

comment:13 Changed 3 weeks ago by cohosh

Actual Points: 1
Note: See TracTickets for help on using tickets.