Skip to content

Conversation

@kingsword09
Copy link
Contributor

@kingsword09 kingsword09 commented Jun 25, 2025

Which issue does this PR close?

Related #6320

Rationale for this change

fix ListOptions list with deleted test

What changes are included in this PR?

Are there any user-facing changes?

@kingsword09 kingsword09 requested a review from suyanhanx as a code owner June 25, 2025 06:22
@dosubot dosubot bot added size:XS This PR changes 0-9 lines, ignoring generated files. releases-note/fix The PR fixes a bug or has a title that begins with "fix" labels Jun 25, 2025
@kingsword09 kingsword09 force-pushed the fix-nodejs-listoptions-test branch from d9a3f93 to f212773 Compare June 25, 2025 06:26
@kingsword09 kingsword09 marked this pull request as draft June 25, 2025 06:50
@kingsword09 kingsword09 force-pushed the fix-nodejs-listoptions-test branch from f212773 to 97486ea Compare June 25, 2025 07:32
@kingsword09
Copy link
Contributor Author

kingsword09 commented Jun 25, 2025

@Xuanwo How do I trigger the COS service test in GitHub Actions? After the merge, when the COS test was triggered, my list with deleted test had issues.

@kingsword09 kingsword09 force-pushed the fix-nodejs-listoptions-test branch from 97486ea to afcd306 Compare June 25, 2025 08:14
@Xuanwo
Copy link
Member

Xuanwo commented Jun 25, 2025

@Xuanwo How do I trigger the COS service test in GitHub Actions? After the merge, when the COS test was triggered, my list with deleted test had issues.

Hi, GitHub doesn't allow forked repositories to trigger tests that require credentials, so this isn't possible. The only way is to become an opendal committer.

According to the cos test, I personally believe it's an issue with the service itself.

@kingsword09 kingsword09 marked this pull request as ready for review June 25, 2025 08:20
@dosubot dosubot bot added size:S This PR changes 10-29 lines, ignoring generated files. and removed size:XS This PR changes 0-9 lines, ignoring generated files. labels Jun 25, 2025
@kingsword09
Copy link
Contributor Author

@Xuanwo How do I trigger the COS service test in GitHub Actions? After the merge, when the COS test was triggered, my list with deleted test had issues.

Hi, GitHub doesn't allow forked repositories to trigger tests that require credentials, so this isn't possible. The only way is to become an opendal committer.

According to the cos test, I personally believe it's an issue with the service itself.

@Xuanwo Then this list_with_deleted test whether you want to take a look again, it always feels a bit wrong,Is it right to list_with(file_path) instead of list_with(parent)?

pub async fn test_list_files_with_deleted(op: Operator) -> Result<()> {
if !op.info().full_capability().list_with_deleted {
return Ok(());
}
let parent = TEST_FIXTURE.new_dir_path();
let file_name = TEST_FIXTURE.new_file_path();
let file_path = format!("{}{}", parent, file_name);
op.write(file_path.as_str(), "1").await?;
// List with deleted should include self too.
let ds = op.list_with(&file_path).deleted(true).await?;
assert_eq!(
ds.len(),
1,
"list with deleted should contain current active file version"
);
op.write(file_path.as_str(), "2").await?;
op.delete(file_path.as_str()).await?;
// This file has been deleted, list with deleted should contain its versions and delete marker.
let mut ds = op.list_with(&file_path).deleted(true).await?;
ds.retain(|de| de.path() == file_path && de.metadata().is_deleted());
assert_eq!(
ds.len(),
1,
"deleted file should be found and only have one"
);
Ok(())
}

@Xuanwo
Copy link
Member

Xuanwo commented Jun 25, 2025

Then this list_with_deleted test whether you want to take a look again, it always feels a bit wrong,Is it right to list_with(file_path) instead of list_with(parent)?

Yes, it's expected to list the file_path itself. We should see the file itself but is deleted.

@Xuanwo Xuanwo merged commit 299a86b into apache:main Jun 25, 2025
67 checks passed
@kingsword09
Copy link
Contributor Author

Then this list_with_deleted test whether you want to take a look again, it always feels a bit wrong,Is it right to list_with(file_path) instead of list_with(parent)?

Yes, it's expected to list the file_path itself. We should see the file itself but is deleted.

Got it, thanks for confirming!

@kingsword09 kingsword09 deleted the fix-nodejs-listoptions-test branch June 25, 2025 12:51
@dosubot dosubot bot added the lgtm This PR has been approved by a maintainer label Jun 25, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

lgtm This PR has been approved by a maintainer releases-note/fix The PR fixes a bug or has a title that begins with "fix" size:S This PR changes 10-29 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants