Feat : add testing fixtures and a basic test

[ocdbytes]

• Added local stack s3 bucket mocking tests for putting and getting the data.

[odesenfans]

A few comments and questions. Mostly, you should document the intent of your tests and use fixtures to clean up the DB.

[odesenfans]

Nitpick: why don't you put this in a dedicated module to avoid annotating every item with #[cfg(test)] ?

[heemankv]

Valid, Moved this code to a orchestrator/src/tests/config.rs
No need to feature flag now.

[odesenfans]

To avoid these unwraps, a better pattern is to do, for each entry (example with prover_client):

let prover_client = self.prover_client.unwrap_or_else(|| build_prover_service(&settings_provider));

and then just pass prover_client to Config::new().

[heemankv]

Valid, unwrap_or_else can readily be used for prover_client for better code pattern.

Upon further inspection :

• settlement_client
• storage_client
• da_client
• database
  are built using async build functions which makes it tricky to create using unwrap_or_else.

We can write something like :

use tokio::runtime::Handle;

// ...

self.settlement_client = Some(self.settlement_client.unwrap_or_else(|| {
    Handle::current().block_on(build_settlement_client(&settings_provider))
}));

But then we'll have unnecessary runtime overhead.

Proposed Solution :

• change sync build functions to use unwrap_or_else.
• handle async build functions in separate assignment using await.

WDYT ?

[odesenfans]

Why is this only used in tests? Is there no equivalent notion when connecting to AWS directly? It would be better to avoid this difference between production and testing, if possible.

[heemankv]

Localstack endpoint URL : http://localhost.localstack.cloud:4566
AWS endpoint URL. : https://s3.{region}.amazonaws.com/{bucket-name}/{key}

We are using localstack for mocking AWS locally.
Localstack requires endpoint_url to get connected.

Connection to AWS doesn't necessarily require the endpoint url as shown here.
Rather, the connection to localstack does as shown here.

Hence this implementation is under the tests flag.
What do you propose as a solution?,
Do you think changing prod code as how tests are written (adding endpoint url on prod just because it's available on tests) is a valid way to go ?

[odesenfans]

But specifying the endpoint is still a possibility on AWS, right? I'm not in favor of "modifying production code", but here it's just that you have 2 different ways of connecting: either you specify the region, bucket key etc or you provide the endpoint directly. It turns out that localstack only supports the endpoint method, sure.

In effect you have two usable connection solutions => you could use an enumeration to support both features, for example giving priority to the endpoint method if someone wants to hardcode it for some reason and then support the default method as a backup. This way, your code is the same for testing vs prod and you support two methods to connect to AWS. WDYT?

[ocdbytes]

Hey @odesenfans
I agree with your implementation and it is more cleaner. I will implement this and push the code.

[odesenfans]

Nitpick: when I see the call to clone(), my first reflex is to check the cost of this operation. I looked at the docs of MongoDB and saw that Client uses an Arc internally, so this is fine. But it is worth adding a comment explaining why this is fine, because for non Arc-based types doing

pub fn client(&self) -> &Client {
  &self.client
}

and then letting the caller decide if he wants to clone is better.

TL;DR, add a comment to explain that Client uses Arc and therefore is cheap to clone.

[heemankv]

Valid and a great catch,
added docs to describe that client uses Arc.

[odesenfans]

How is this change related to the PR?

[heemankv]

This is a valid concern,
While the tests for Job_controller were being written it was realised that we are not using it anymore and hence it can be deleted, we agree, ideally this is to be done on a separate PR.

Proposed Solution :

• Have stricter decouples of PR from next time.
• Keep the commit with this PR.
• Mention removal of job_controller in the CHANGELOG.

WDYT ?

[odesenfans]

You can keep it in this PR, but mention it in the changelog and in the PR description.

[ocdbytes]

sure

[odesenfans]

Careful that you'll need to add every future collection in here as well. It might be better to just drop every collection under the orchestrator DB.

[heemankv]

Valid, changed to dropping all the collections for now.

• Added comment to how to change to specific collection.

[odesenfans]

So this is a sanity check to see that putting and getting to/from S3 is working, right? Don't hesitate to document what your tests intend to do, that helps for review.

[heemankv]

Valid, Added docs for this test.

[odesenfans]

Use a fixture to start with a fresh DB and an already built config.

[heemankv]

Valid,
We had used fixture in the initial stages of tests but we soon realised that we require more customisability like mocking only da_client while writing tests, hence we went with the Builder Design Pattern to achieve the same.

Moreover :

• drop_database is an async fn, we didn't find this to be a valid enough reason to implement Drop on Config.

Proposed Solution :

• we add drop_database as a part of TestConfigBuilder.build() so that we drop the database once before each test-case runs, which gives same result as dropping database after each test-case.

WDYT ?

[odesenfans]

Several things to say here.

1. Regarding when to drop the DB, I believe it's best to drop it when the test starts and not after it finishes. This way, you can inspect the DB to understand what went wrong.
2. Fixture vs manual call: While you're going to have multiple test configs, you're only going to use a few. So defining a few fixtures with the right configuration options on the test builder that return the test config is probably the right thing to do. This enables finer-grained control on when to reset the DB (once for a group of tests? For each test?) depending on what you want.
3. We didn't talk about test concurrency here, but if you let all your tests execute in parallel you have a strong risk of dropping the DB in the middle of another test. Do you have a solution for this?

[ocdbytes]

1. Yes so we will do that in the beginning of the tests only.
2. Will have to think about that can you give me an example on this if possible ?
3. We were also discussing about this few days back. We will have to come up on this solution but currently we don't have one yet. We were thinking to make collections test wise or databases for different tests something like that.

[apoorvsadana]

To add on (3), the major blocker for concurrency would be DB or AWS localstack. We haven't finalized the implementation yet but it shouldn't be difficult to add on the current implementation. As @ocdbytes mentioned, we were looking at mostly using different databases for each test case and different localstack accounts. I think the changes should be localized to the builder itself but the idea was to focus on this more when we see test cases taking >15mins or so. wdyt?

[odesenfans]

Not sure what "Dev might want to pass an Arc manually." means.

[ocdbytes]

will remove this comment : "Dev might want to pass an Arc manually.". It may create confusion for someone.

[EvolveArt]

smol review