Optimisation: parallel test runs using testcontainer #96

heemankv · 2024-08-23T06:18:08Z

No description provided.

coveralls · 2024-08-23T07:01:19Z

coverage: 61.038% (-3.6%) from 64.606%
when pulling 1d79902 on optimisation/parallel-test-runs
into f84ba41 on main.

heemankv

Completed Self Review, and implemented according improvements along the way.
Over to you @apoorvsadana

apoorvsadana · 2024-09-02T18:23:48Z

.env.test

@@ -8,17 +8,20 @@ PORT=3000
 AWS_ACCESS_KEY_ID="AWS_ACCESS_KEY_ID"
 AWS_SECRET_ACCESS_KEY="AWS_SECRET_ACCESS_KEY"
 AWS_REGION="us-east-1"
+AWS_S3_BUCKET_REGION="us-east-1"


why can't we just reuse AWS_REGION

apoorvsadana · 2024-09-02T18:24:05Z

.env.test

i don't think this should be an env var

apoorvsadana · 2024-09-02T18:27:24Z

crates/orchestrator/Cargo.toml

@@ -46,6 +46,7 @@ num-bigint = { workspace = true }
 num-traits = { workspace = true }
 omniqueue = { workspace = true, optional = true }
 prover-client-interface = { workspace = true }
+reqwest = { version = "0.11", features = ["blocking"] }


should import from workspace ideally

Valid, resolved.

apoorvsadana · 2024-09-02T18:27:44Z

crates/orchestrator/Cargo.toml

@@ -69,9 +70,12 @@ default = ["ethereum", "with_mongodb", "with_sqs"]
 ethereum = ["ethereum-da-client"]
 with_mongodb = ["mongodb"]
 with_sqs = ["omniqueue"]
+mongodb = ["dep:mongodb"]


we already have with_mongodb, why do we need this?

apoorvsadana · 2024-09-02T18:30:48Z

crates/orchestrator/src/alerts/aws_sns/mod.rs

        let sns_region = get_env_var_or_panic("AWS_SNS_REGION");
+        let topic_arn = get_env_var_or_panic("AWS_SNS_ARN");
        let config = aws_config::from_env().region(Region::new(sns_region)).load().await;


we should actually refactor this now on main because now we send aws_config from the root, can you pick this up?

Valid, added to #104

apoorvsadana · 2024-09-02T19:02:47Z

crates/orchestrator/src/tests/config.rs

+    }
+
+    pub async fn testcontainer_mongo_database(mut self) -> TestConfigBuilder {
+        let (node, database) = mongodb_testcontainer_setup().await;


i would call the setup functions during build and not during these intermediate builder functions

Builders only ensures that if anything is not built then add a mock client to it, rest all the functions be it testcontainers or adding creates/takes services and stores in the builder,
if we want to build them in .build() function then we'll have to maintain extra variables to manage state of what needs to be built and what needs to be mocked, I don't think it's worth the effort.
WDYT ?

discussed in person

apoorvsadana · 2024-09-02T19:04:16Z

crates/orchestrator/src/tests/config.rs

+        .with_stdout_level(log::Level::Info)
+        .with_stdout_level(log::Level::Debug)
+        .with_stdout_level(log::Level::Error)
+        .with_stdout_level(log::Level::Trace)
+        .with_stdout_level(log::Level::Warn)
+        .with_stderr_level(log::Level::Info)
+        .with_stderr_level(log::Level::Debug)
+        .with_stderr_level(log::Level::Error)
+        .with_stderr_level(log::Level::Trace)
+        .with_stderr_level(log::Level::Warn);


repeated + trace includes everything afaik

Alright, only kept Trace,
I am not sure how to create a file scoped logger and use it here, can you share an example ?

I am not sure how to create a file scoped logger

I didn't understand, wdym by a scoped logger?

apoorvsadana · 2024-09-02T19:08:14Z

crates/orchestrator/src/tests/testcontainers/mod.rs

+
+    let urll = sqs_client.get_queue_url().queue_name(JOB_PROCESSING_QUEUE.to_string()).send().await.unwrap();
+
+    println!("Sdsdfdf {:?}", urll);


Suggested change

println!("Sdsdfdf {:?}", urll);

Valid, removed.

apoorvsadana · 2024-09-02T19:09:12Z

crates/orchestrator/src/tests/testcontainers/mod.rs

i am not sure if this is very helpful because there's no assurity that different cases will actually run in parallel

didn't get you, could you elaborate ?

I assume the test cases here are meant to check if parallel testing works fine. For parallel testing, we create multiple dummy cases (random UUIDs each) for each test expecting these cases will run in ||. However, I think it's not necessary they actually run in || so these cases might not be relevant.

apoorvsadana · 2024-09-02T19:09:56Z

crates/orchestrator/src/tests/workers/update_state/mod.rs

-                .with(eq(job.internal_id.to_string()), eq(JobType::StateTransition))
-                .returning(|_, _| Ok(None));
-        }
+        // for job in completed_jobs {


can we delete this comment? how was the code working with this commented

The test case was expecting many instances of get_job_by_internal_id_and_type because of the loop, which were not being used since only the first one was being used we corrected it to only expect for 1st element of the vector. Removed the comment.

heemankv requested a review from apoorvsadana August 23, 2024 06:18

heemankv self-assigned this Aug 23, 2024

heemankv changed the title ~~Optimisation/parallel test runs~~ Optimisation: parallel test runs using testcontainer Aug 23, 2024

heemankv changed the title ~~Optimisation: parallel test runs using testcontainer~~ Optimisation: parallel test runs using testcontainer Aug 23, 2024

heemankv force-pushed the optimisation/parallel-test-runs branch from 96f8038 to bfbce36 Compare August 26, 2024 13:51

heemankv added 16 commits August 27, 2024 11:35

update: testcontainer for mongo

09fa36b

update: working testcontainers for localstack and mongodb

c6648f1

update: new creator for SqsQueue

d2cb7a2

docs: update changelog

bec83b3

update: handling trailing slash

1b8d830

update: working localstack sqs setup

7767d8b

update: working localstack s3 tests

754aa25

update: init working test with testcontainer

905d9f3

chore: lint fixes

081fee9

update: parallel tests draft #1

dbe12a9

chores: lint fixes

cfd0532

update: fixing test fns according to testcontainers

df5c21f

update: fixing tests for parallelisation

dd7927c

update: create both queues for testing

67e7af7

update: working test_process_job_works

99f3407

update: AWS related changes for test

6c9b32f

heemankv force-pushed the optimisation/parallel-test-runs branch from c1e789e to 6c9b32f Compare August 27, 2024 06:29

heemankv added 7 commits August 27, 2024 13:56

fix: CI test

98253ad

update: making CI parallel

528226d

update: remove localstack service from coverage CI

5cadb36

update: fixing clippy lint

84e3867

update: printing time and port for testcontainers

f0849c6

chore: new logs addition

6043941

chore: new logs addition

097bf91

heemankv added 11 commits August 29, 2024 19:36

chore: removed no capture from CI

aaeede1

update: config as param

7f11668

update: retry localstack attempts

4a60d81

Merge branch 'main' into optimisation/parallel-test-runs

2f5d674

update: lint fixes

ec4a5ac

update: coverage action

4a35737

lint: taplo lint fixes

6c9fc0f

update: removing logs from CI

23845b9

chore: remove env_logger

f374bd1

chore: docs

269493a

chore: lint updates

08b6b82

heemankv commented Sep 1, 2024

View reviewed changes

chores: cleanup

82fa328

apoorvsadana mentioned this pull request Sep 2, 2024

Update/queue builder #94

Closed

heemankv added 2 commits September 2, 2024 18:29

update: adoption of sns and testbuilderconfig

1d79902

chore: lint fixes

e148137

apoorvsadana reviewed Sep 2, 2024

View reviewed changes

heemankv mentioned this pull request Sep 3, 2024

Add code from parallel test cases branch to Main #104

Closed

heemankv added 3 commits September 3, 2024 12:56

update: PR review fixes

f5e4ddb

update: PR review fixes

70ddc48

chore: lints

a75870c

heemankv mentioned this pull request Sep 4, 2024

Better Alerts #110

Closed

heemankv added the deferred This PR is ready but should not be merged at the current time, contact manager to know more. label Sep 11, 2024

apoorvsadana closed this Sep 23, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Optimisation: parallel test runs using testcontainer #96

Optimisation: parallel test runs using testcontainer #96

heemankv commented Aug 23, 2024

coveralls commented Aug 23, 2024 •

edited

Loading

heemankv left a comment

apoorvsadana Sep 2, 2024

apoorvsadana Sep 2, 2024

apoorvsadana Sep 2, 2024

heemankv Sep 3, 2024

apoorvsadana Sep 2, 2024

apoorvsadana Sep 2, 2024

heemankv Sep 3, 2024

apoorvsadana Sep 2, 2024

heemankv Sep 3, 2024

apoorvsadana Sep 3, 2024

apoorvsadana Sep 2, 2024

heemankv Sep 3, 2024 •

edited

Loading

apoorvsadana Sep 3, 2024

apoorvsadana Sep 2, 2024

heemankv Sep 3, 2024

apoorvsadana Sep 2, 2024

heemankv Sep 3, 2024

apoorvsadana Sep 3, 2024

apoorvsadana Sep 2, 2024

heemankv Sep 3, 2024


		let urll = sqs_client.get_queue_url().queue_name(JOB_PROCESSING_QUEUE.to_string()).send().await.unwrap();

		println!("Sdsdfdf {:?}", urll);

Optimisation: parallel test runs using testcontainer #96

Optimisation: parallel test runs using testcontainer #96

Conversation

heemankv commented Aug 23, 2024

coveralls commented Aug 23, 2024 • edited Loading

heemankv 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

heemankv Sep 3, 2024 • edited 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

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

coveralls commented Aug 23, 2024 •

edited

Loading

heemankv Sep 3, 2024 •

edited

Loading