Skip to content

Conversation

@boraarslan
Copy link
Collaborator

No description provided.

@guilload
Copy link
Member

Please, add more asserts in your tests. Hitting the desired code path is not enough...


#[tokio::test]
async fn test_cmd_garbage_collect_spares_files_within_grace_period() -> Result<()> {
async fn test_cmd_garbage_collect_spares_files_within_grace_period() {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
async fn test_cmd_garbage_collect_spares_files_within_grace_period() {
async fn test_garbage_collect_index_cli() {

data_dir: None,
};

garbage_collect_index_cli(args).await.unwrap();
Copy link
Member

Choose a reason for hiding this comment

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

This needs to be followed by an assert...

let args = GarbageCollectIndexArgs {
config_uri: test_env.config_uri.clone(),
index_id: index_id.clone(),
grace_period: Duration::from_secs(2),
Copy link
Member

Choose a reason for hiding this comment

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

We can probably shorten this to 1s


// Wait for grace period.
// TODO: edit split update timestamps and remove this sleep.
sleep(Duration::from_secs(3)).await;
Copy link
Member

Choose a reason for hiding this comment

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

and sleep here for 2s.


Ok(())
garbage_collect_index_cli(args).await.unwrap();
assert_eq!(split_path.exists(), false);
Copy link
Member

Choose a reason for hiding this comment

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

We need an assert on the number of splits in the metastore.

@boraarslan boraarslan enabled auto-merge (squash) November 7, 2022 17:45
@boraarslan boraarslan merged commit 3e54c37 into main Nov 7, 2022
@boraarslan boraarslan deleted the bora/refactor-cli-tests/gc-within-grace branch November 7, 2022 18:05
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