POC for Integration Testing by TheRipperoni · Pull Request #62 · blacksky-algorithms/rsky

TheRipperoni · 2025-02-15T02:43:20Z

Summary

Implementation of Integration Testing for the RSky-PDS APIs. As of now, there are only unit tests for small pieces of logic and nothing that would cover an endpoint in itself. This adds several integration tests, and supplies the starting framework to be used for writing API integration tests going forward.

Rocket method will be moved into the library rsky-pds package in order to properly test, where the main would simply call it once.

Strategy is

Utilize the testcontainer crate in order to create a Postgres container for the integration tests
Use diesel_migrations to setup necessary database changes
Create an instance of the PDS connected to a testcontainer to perform API level testing

Components that may need either mocking, or creative solutions (Not yet implemented)

The PLC Directory calls. One option for further investigation later involves creating a new testcontainer for a love PLC Directory
Calls to the BSky App Server.

Other Updates

For CreateInviteCode and CreateAccount, created new functions that utilize the DBPool from Rocket rather than using the deprecated EstablishConnection. This is both required for testing, and best practice for functions in general. Chose to implement the methods as "v2" so other calls utilizing the old functions are undisturbed. When everything eventually shifts to the v2 calls, can simply delete the unused old, and rename.

Related Issues

#49
#24

Changes

Feature implementation
Bug fix
Documentation update
Other (please specify): Testing Implmentation and Framework

Checklist

I have tested the changes (including writing unit tests).
I confirm that my implementation aligns with the canonical Typescript implementation and/or atproto spec
I have updated relevant documentation.
I have formatted my code correctly
I have provided examples for how this code works or will be used

# Conflicts: # rsky-pds/src/main.rs

TheRipperoni · 2025-02-15T17:16:45Z

@rudyfraser Is an approach like this something on the table?

rudyfraser · 2025-02-16T00:49:09Z

@rudyfraser Is an approach like this something on the table?

Yeah, definitely. As long as tests can still run concurrently and the DBs are torn down after the tests complete I'm good with this approach.

TheRipperoni · 2025-02-18T00:07:36Z

Having to address some tech debt related to establish_connection() to properly implement

TheRipperoni · 2025-02-18T01:30:22Z

Will clean up and make a new pull request. Only thing last to check is how it behaves on the Github pipeline

# Conflicts: # rsky-pds/src/main.rs

TheRipperoni · 2025-02-20T02:08:40Z

Came across integration-test issue as service is dependent on environment variables to startup. @rudyfraser is it possible for us to pass environment variables necessary to start an rsky-pds in the github action?

The specific erroring for this one is on "PDS_ADMIN_PASS"

Should resolve issues in #62

rudyfraser · 2025-02-25T21:07:34Z

I added a bunch of environment vars to the GitHub workflow that should fix this but let me know if it isn't squared away.

Updated pipeline

TheRipperoni · 2025-02-26T01:57:45Z

I added a bunch of environment vars to the GitHub workflow that should fix this but let me know if it isn't squared away.

Thanks! It looks like build is failing testing when it uses the secrets field specifically? Unsure why that is. I passed it some hard values for the time being to validate that.

Other than that, can confirm that test containers is working. Going to cleanup the PR a bit and then move it into "Ready for Review"

TheRipperoni · 2025-03-04T23:32:04Z

Any feedback on this one?

TheRipperoni · 2025-03-04T23:33:16Z

If this is ready to go before OAuth, we can have integration testing for oauth ideally

rudyfraser · 2025-03-04T23:34:51Z

Missed this, sorry. Thanks for the ping. Will review tonight.

rudyfraser

The tests look good. However, I’m not a fan of the foo_v2 pattern (I stopped commenting on it midway).

Instead, I think AccountManager should be updated from:

#[derive(Clone)]
pub struct AccountManager {}

to:

#[derive(Clone)]
pub struct AccountManager {
    pub db: Arc<DbConn>
}

Then, update the functions to be instance methods (&self) instead of static functions.

This aligns with the TypeScript implementation and helps keep things DRY.

rudyfraser · 2025-03-05T23:29:47Z

rsky-pds/src/account_manager/helpers/account.rs

What's the reasoning behind the foo_v2 pattern? Any reason to not refactor the code to use DbConn entirely? That's where I'd like things to go in any case and get rid of "establish_connection" where possible. It's a bit of technical debt.

I was using v2 to avoid having to do a total rewrite of the connection pattern yet. Plan was to move all to v2, then remove the old method, and then just remove v2

rudyfraser · 2025-03-05T23:54:35Z

rsky-pds/src/account_manager/helpers/auth.rs

+pub async fn store_refresh_token_v2(
+    payload: RefreshToken,
+    app_password_name: Option<String>,
+    db: &DbConn,
+) -> Result<()> {
+    use crate::schema::pds::refresh_token::dsl as RefreshTokenSchema;
+
+    let exp = from_micros_to_utc((payload.exp.as_millis() / 1000) as i64);
+
+    db.run(move |conn| {
+        insert_into(RefreshTokenSchema::refresh_token)
+            .values((
+                RefreshTokenSchema::id.eq(payload.jti),
+                RefreshTokenSchema::did.eq(payload.sub),
+                RefreshTokenSchema::appPasswordName.eq(app_password_name),
+                RefreshTokenSchema::expiresAt.eq(format!("{}", exp.format(RFC3339_VARIANT))),
+            ))
+            .on_conflict_do_nothing() // E.g. when re-granting during a refresh grace period
+            .execute(conn)
+    })
+    .await?;
+


Similar question here on need for v2

I was using v2 to avoid having to do a total rewrite of the connection pattern yet. Plan was to move all to v2, then remove the old method, and then just remove v2

I'd rather not have the transition period. Let me know if you'd like me to take on the migration to the v2 pattern.

I can take this up

Can have it updated by end of the weekend

rudyfraser · 2025-03-05T23:56:30Z

rsky-pds/src/account_manager/helpers/invite.rs

Avoid foo_v2 pattern

rudyfraser · 2025-03-05T23:57:30Z

rsky-pds/src/account_manager/helpers/password.rs

Avoid foo_v2 pattern

rudyfraser · 2025-03-05T23:58:21Z

rsky-pds/src/account_manager/helpers/repo.rs

+pub async fn update_root_v2(did: String, cid: Cid, rev: String, db: &DbConn) -> Result<()> {
+    // @TODO balance risk of a race in the case of a long retry
+    use crate::schema::pds::repo_root::dsl as RepoRootSchema;
+
+    let now = rsky_common::now();
+
+    db.run(move |conn| {
+        insert_into(RepoRootSchema::repo_root)
+            .values((
+                RepoRootSchema::did.eq(did),
+                RepoRootSchema::cid.eq(cid.to_string()),
+                RepoRootSchema::rev.eq(rev.clone()),
+                RepoRootSchema::indexedAt.eq(now),
+            ))
+            .on_conflict(RepoRootSchema::did)
+            .do_update()
+            .set((
+                RepoRootSchema::cid.eq(cid.to_string()),
+                RepoRootSchema::rev.eq(rev),
+            ))
+            .execute(conn)
+    })
+    .await?;
+
+    Ok(())
+}


avoid foo_v2

rudyfraser · 2025-03-06T00:19:47Z

rsky-pds/tests/common/mod.rs

+    client
+        .post("/xrpc/com.atproto.server.createAccount")
+        .header(ContentType::JSON)
+        .header(Header::new("Authorization", get_admin_token()))
+        .body(account_input.to_string())
+        .dispatch()
+        .await;
+
+    ("dummyemail@rsky.com".to_string(), "password".to_string())


What happens if there's an error here in creating the account (for example, generating a DID)?

I can add an assert. Currently it will cause the tests utilizing the function to fail as the account won't exist.

From what I can see it's only used in test_create_session. And if it isn't creating an account then what is it actually doing?

It does create an account, if it was to fail currently the account wouldn't exist at all tests would fail

Testing specific function to make tests easier to write

TheRipperoni · 2025-03-07T19:06:18Z

@rudyfraser Left some comments

TheRipperoni · 2025-03-09T09:46:20Z

The tests look good. However, I’m not a fan of the foo_v2 pattern (I stopped commenting on it midway).

Instead, I think AccountManager should be updated from:
#[derive(Clone)]

pub struct AccountManager {}
to:
#[derive(Clone)]

pub struct AccountManager {

    pub db: Arc<DbConn>

}
Then, update the functions to be instance methods (&self) instead of static functions.

This aligns with the TypeScript implementation and helps keep things DRY.

Will have an update for this later today

TheRipperoni · 2025-03-09T20:12:47Z

The tests look good. However, I’m not a fan of the foo_v2 pattern (I stopped commenting on it midway).

Instead, I think AccountManager should be updated from:
#[derive(Clone)]
pub struct AccountManager {}
to:
#[derive(Clone)]
pub struct AccountManager {
    pub db: Arc<DbConn>
}
Then, update the functions to be instance methods (&self) instead of static functions.

This aligns with the TypeScript implementation and helps keep things DRY.

I need to check how this plays with the Rocket Fairing specifically

TheRipperoni · 2025-03-10T05:52:19Z

Realized changing AccountManager to a state like that has some knock on effects as currently it is used in LocalViewer, which will mean LocalViewer will need updates and changes as well

rudyfraser · 2025-03-11T19:59:12Z

Realized changing AccountManager to a state like that has some knock on effects as currently it is used in LocalViewer, which will mean LocalViewer will need updates and changes as well

Does this mean you'd like to hold off on making the changes or that you're in the middle of changes but will take some more time?

TheRipperoni · 2025-03-12T19:19:47Z

Realized changing AccountManager to a state like that has some knock on effects as currently it is used in LocalViewer, which will mean LocalViewer will need updates and changes as well

Does this mean you'd like to hold off on making the changes or that you're in the middle of changes but will take some more time?

I was in the middle of making the changes, but realized that there are some key differences in the typescript reference.

One of them is that because it uses SQLite, it doesn't use a connection pool to establish the connection. Because LocalViewer is a Read/Write Lock, but the db connection pool is managed by Rocket, I'd like to move the function logic into the SharedLocalViewer instead and have the SharedLocalViewer be a request guard that utilizes the local viewer state.

My main concern is it bloats the pull request into even more changes. If that's acceptable, I can do it, but it will have affects

rudyfraser · 2025-03-15T02:15:30Z

Instead of naming the new functions "foo_v2", please rename the old function "bar_legacy" and add the #[deprecated] attribute to them.

TheRipperoni · 2025-03-15T10:33:37Z

Instead of naming the new functions "foo_v2", please rename the old function "bar_legacy" and add the #[deprecated] attribute to them.

Can do, will update later today for review

TheRipperoni · 2025-03-15T18:03:10Z

@rudyfraser Renamed v2 and marked old functions as legacy

# Conflicts: # .gitignore

TheRipperoni and others added 5 commits February 12, 2025 16:48

checkpoint

5afdca4

temp

675a947

First Rough Draft for integration testing support

cf7ef92

Merge remote-tracking branch 'origin/main' into localIntegrationTesting

eba714f

# Conflicts: # rsky-pds/src/main.rs

Draft of POC for integration testing

1eec644

checkpoint

103de17

checkpoint

7b2681a

TheRipperoni marked this pull request as ready for review February 18, 2025 00:59

TheRipperoni marked this pull request as draft February 18, 2025 01:00

Refactored codebase to handle db connection from fairing

f623f68

TheRipperoni and others added 3 commits February 17, 2025 22:40

Added create session to the integration tests

165da77

Merge remote-tracking branch 'origin/main' into localIntegrationTesting

f4dacf7

# Conflicts: # rsky-pds/src/main.rs

Added identity apis

c6ae2dc

TheRipperoni changed the title ~~POC for Implementation Testing~~ POC for Integration Testing Feb 22, 2025

rudyfraser added a commit that referenced this pull request Feb 24, 2025

Update rust.yml

3b66643

Should resolve issues in #62

TheRipperoni added 9 commits February 25, 2025 13:32

Pinned sha2 dependency to specific version

43e45d3

Merge branch 'main' into integrationTesting

8138149

Added handle domains to the pipeline variables

81bcf3a

changed secret keys in pipeline for test

dae6f58

Removed .idea directory from the git

d6355bc

Updated pipeline

fixed comment

dbf4755

trying different variable

efab30d

trying different variable

f7380cd

trying different variable

e9cb989

TheRipperoni added 2 commits February 25, 2025 16:40

trying different variable

4b513bd

Fixed formatting

6fb47f5

Added some documentation for the testing methods

78b1ff1

TheRipperoni marked this pull request as ready for review February 26, 2025 02:08

rudyfraser requested changes Mar 6, 2025

View reviewed changes

Renamed v2 and marked old functions as legacy and deprecated

a79deb8

Merge remote-tracking branch 'origin/main' into BranchName

7f9766b

# Conflicts: # .gitignore

rudyfraser approved these changes Mar 16, 2025

View reviewed changes

rudyfraser merged commit 543fb2d into blacksky-algorithms:main Mar 16, 2025
4 checks passed

Uh oh!

Conversation

TheRipperoni commented Feb 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Related Issues

Changes

Checklist

Uh oh!

TheRipperoni commented Feb 15, 2025

Uh oh!

rudyfraser commented Feb 16, 2025

Uh oh!

TheRipperoni commented Feb 18, 2025

Uh oh!

TheRipperoni commented Feb 18, 2025

Uh oh!

TheRipperoni commented Feb 20, 2025

Uh oh!

rudyfraser commented Feb 25, 2025

Uh oh!

TheRipperoni commented Feb 26, 2025

Uh oh!

TheRipperoni commented Mar 4, 2025

Uh oh!

TheRipperoni commented Mar 4, 2025

Uh oh!

rudyfraser commented Mar 4, 2025

Uh oh!

rudyfraser left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TheRipperoni commented Mar 7, 2025

Uh oh!

TheRipperoni commented Mar 9, 2025

Uh oh!

TheRipperoni commented Mar 9, 2025

Uh oh!

TheRipperoni commented Mar 10, 2025

Uh oh!

rudyfraser commented Mar 11, 2025

Uh oh!

TheRipperoni commented Mar 12, 2025

Uh oh!

rudyfraser commented Mar 15, 2025

Uh oh!

TheRipperoni commented Mar 15, 2025

Uh oh!

TheRipperoni commented Mar 15, 2025

TheRipperoni commented Feb 15, 2025 •

edited

Loading

rudyfraser left a comment •

edited

Loading