Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

test(kitsune): add integration test #100

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

jost-s
Copy link
Contributor

@jost-s jost-s commented Feb 5, 2025

No description provided.

@jost-s jost-s force-pushed the test/add-kitsune-integration-test branch 3 times, most recently from 0c99c4b to 1faba6d Compare February 7, 2025 03:00
@jost-s jost-s marked this pull request as ready for review February 7, 2025 03:01
@jost-s jost-s requested a review from a team February 7, 2025 03:01
@jost-s jost-s force-pushed the test/add-kitsune-integration-test branch from 1faba6d to 3b51f46 Compare February 7, 2025 03:09
}

#[derive(Debug)]
struct HolochainSpaceHandler;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would prefer to not have Holochain* names in here, hopefully we've done a reasonable job of decoupling here

let (ops_2, op_ids_2) = create_op_list(100);
space_2
.op_store()
.process_incoming_ops(ops_2.clone())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Process incoming ops will make the ops available for "what's new" but there will be no DHT diff happening unless gossip is informed about these ops through the space with "inform_ops_stored". That will work fine for this test because all the ops are new. Just wanted to mention that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I had that here but noticed it didn't make a diff(erence).

Comment on lines 137 to 141
let kitsune = kitsune_builder
.build(kitsune_handler.clone())
.await
.unwrap();
kitsune.space(TEST_SPACE_ID).await.unwrap()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're dropping the top level kitsune instance here. We probably aren't cleaning everything up properly yet, but eventually this will shut everything down so your test won't work.

},
})
.unwrap();
kitsune_builder.op_store = MemOpStoreFactory::create();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't replace a factory after you've already set the default config... you end up with a config for the wrong factory.

let mut kitsune_builder = Builder {
    op_store: MemOpStoreFactory::create(),
    ..default_builder()
}.with_default_config().unwrap();

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, the MemOpStoreFactory is the default, so you don't need to do this at all.

Copy link
Collaborator

@neonphog neonphog left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

running 1 test
test test::two_node_gossip ... ok

test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 10.01s

I wish cargo test would give times for the individual tests. Luckily, since there's only one in there, we can see that it is 10s! Not entirely unreasonable.

Couple cleanup suggestions.

@jost-s jost-s force-pushed the test/add-kitsune-integration-test branch from 3b51f46 to 17f5adb Compare February 11, 2025 13:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants