You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@opendal.apache.org by "Xuanwo (via GitHub)" <gi...@apache.org> on 2023/12/26 13:01:44 UTC
[PR] fix: Add tests for listing recursively on not supported services [incubator-opendal]
Xuanwo opened a new pull request, #3826:
URL: https://github.com/apache/incubator-opendal/pull/3826
(no comment)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: Add tests for listing recursively on not supported services [incubator-opendal]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #3826:
URL: https://github.com/apache/incubator-opendal/pull/3826#discussion_r1436463877
##########
core/tests/behavior/list.rs:
##########
Review Comment:
Do you think this is a blocker for merging the PR? Feel free to make improvements :love_letter:, but I'm okay with the current version.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: Add tests for listing recursively on not supported services [incubator-opendal]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #3826:
URL: https://github.com/apache/incubator-opendal/pull/3826#discussion_r1436457403
##########
core/tests/behavior/list.rs:
##########
Review Comment:
Would you like to make your message more specific?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: Add tests for listing recursively on not supported services [incubator-opendal]
Posted by "suyanhanx (via GitHub)" <gi...@apache.org>.
suyanhanx commented on code in PR #3826:
URL: https://github.com/apache/incubator-opendal/pull/3826#discussion_r1436464436
##########
core/tests/behavior/list.rs:
##########
Review Comment:
Just a suggestion, not a blocker. :-D
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: Add tests for listing recursively on not supported services [incubator-opendal]
Posted by "suyanhanx (via GitHub)" <gi...@apache.org>.
suyanhanx commented on code in PR #3826:
URL: https://github.com/apache/incubator-opendal/pull/3826#discussion_r1436461421
##########
core/tests/behavior/list.rs:
##########
Review Comment:
Like the scenario above, every message is just `dir should only return empty`,
When running the test, it is hard to distinguish which step had the problem.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: Add tests for listing recursively on not supported services [incubator-opendal]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #3826:
URL: https://github.com/apache/incubator-opendal/pull/3826#discussion_r1436455034
##########
core/tests/behavior/list.rs:
##########
@@ -243,15 +243,51 @@ pub async fn test_list_empty_dir(op: Operator) -> Result<()> {
op.create_dir(&dir).await.expect("write must succeed");
+ // List "dir/" should return empty object.
let mut obs = op.lister(&dir).await?;
let mut objects = HashMap::new();
while let Some(de) = obs.try_next().await? {
objects.insert(de.path().to_string(), de);
}
- debug!("got objects: {:?}", objects);
+ assert_eq!(objects.len(), 0, "dir should only return empty");
+ // List "dir" should return "dir/".
+ let mut obs = op.lister(dir.trim_end_matches('/')).await?;
+ let mut objects = HashMap::new();
+ while let Some(de) = obs.try_next().await? {
+ objects.insert(de.path().to_string(), de);
+ }
+ assert_eq!(objects.len(), 1, "dir should only return empty");
Review Comment:
addressed.
##########
core/tests/behavior/list.rs:
##########
@@ -243,15 +243,51 @@ pub async fn test_list_empty_dir(op: Operator) -> Result<()> {
op.create_dir(&dir).await.expect("write must succeed");
+ // List "dir/" should return empty object.
let mut obs = op.lister(&dir).await?;
let mut objects = HashMap::new();
while let Some(de) = obs.try_next().await? {
objects.insert(de.path().to_string(), de);
}
- debug!("got objects: {:?}", objects);
+ assert_eq!(objects.len(), 0, "dir should only return empty");
+ // List "dir" should return "dir/".
+ let mut obs = op.lister(dir.trim_end_matches('/')).await?;
+ let mut objects = HashMap::new();
+ while let Some(de) = obs.try_next().await? {
+ objects.insert(de.path().to_string(), de);
+ }
+ assert_eq!(objects.len(), 1, "dir should only return empty");
+ assert_eq!(
+ objects[&dir].metadata().mode(),
+ EntryMode::DIR,
+ "given dir should exist and must be dir"
+ );
+
+ // List "dir/" should return empty object.
+ let mut obs = op.lister_with(&dir).recursive(true).await?;
+ let mut objects = HashMap::new();
+ while let Some(de) = obs.try_next().await? {
+ objects.insert(de.path().to_string(), de);
+ }
assert_eq!(objects.len(), 0, "dir should only return empty");
+ // List "dir" should return "dir/".
+ let mut obs = op
+ .lister_with(dir.trim_end_matches('/'))
+ .recursive(true)
+ .await?;
+ let mut objects = HashMap::new();
+ while let Some(de) = obs.try_next().await? {
+ objects.insert(de.path().to_string(), de);
+ }
+ assert_eq!(objects.len(), 1, "dir should only return empty");
Review Comment:
addressed
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: Add tests for listing recursively on not supported services [incubator-opendal]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo merged PR #3826:
URL: https://github.com/apache/incubator-opendal/pull/3826
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: Add tests for listing recursively on not supported services [incubator-opendal]
Posted by "suyanhanx (via GitHub)" <gi...@apache.org>.
suyanhanx commented on code in PR #3826:
URL: https://github.com/apache/incubator-opendal/pull/3826#discussion_r1436449733
##########
core/tests/behavior/list.rs:
##########
@@ -243,15 +243,51 @@ pub async fn test_list_empty_dir(op: Operator) -> Result<()> {
op.create_dir(&dir).await.expect("write must succeed");
+ // List "dir/" should return empty object.
let mut obs = op.lister(&dir).await?;
let mut objects = HashMap::new();
while let Some(de) = obs.try_next().await? {
objects.insert(de.path().to_string(), de);
}
- debug!("got objects: {:?}", objects);
+ assert_eq!(objects.len(), 0, "dir should only return empty");
+ // List "dir" should return "dir/".
+ let mut obs = op.lister(dir.trim_end_matches('/')).await?;
+ let mut objects = HashMap::new();
+ while let Some(de) = obs.try_next().await? {
+ objects.insert(de.path().to_string(), de);
+ }
+ assert_eq!(objects.len(), 1, "dir should only return empty");
Review Comment:
Here should be `only return the dir itself`?
##########
core/src/types/operator/operator.rs:
##########
@@ -1113,6 +1175,9 @@ impl Operator {
///
/// ## List entries with metakey for more metadata
///
+ /// Some storage services like `s3` could return more metadata like `content-length` and
+ /// `last-modified`. By using `metakey`, we can fetch those metadata without an extra `stat` call.
Review Comment:
Perhaps we could add some information about the default behavior without metakey here?
This makes it easier to understand.
##########
core/tests/behavior/list.rs:
##########
@@ -243,15 +243,51 @@ pub async fn test_list_empty_dir(op: Operator) -> Result<()> {
op.create_dir(&dir).await.expect("write must succeed");
+ // List "dir/" should return empty object.
let mut obs = op.lister(&dir).await?;
let mut objects = HashMap::new();
while let Some(de) = obs.try_next().await? {
objects.insert(de.path().to_string(), de);
}
- debug!("got objects: {:?}", objects);
+ assert_eq!(objects.len(), 0, "dir should only return empty");
+ // List "dir" should return "dir/".
+ let mut obs = op.lister(dir.trim_end_matches('/')).await?;
+ let mut objects = HashMap::new();
+ while let Some(de) = obs.try_next().await? {
+ objects.insert(de.path().to_string(), de);
+ }
+ assert_eq!(objects.len(), 1, "dir should only return empty");
+ assert_eq!(
+ objects[&dir].metadata().mode(),
+ EntryMode::DIR,
+ "given dir should exist and must be dir"
+ );
+
+ // List "dir/" should return empty object.
+ let mut obs = op.lister_with(&dir).recursive(true).await?;
+ let mut objects = HashMap::new();
+ while let Some(de) = obs.try_next().await? {
+ objects.insert(de.path().to_string(), de);
+ }
assert_eq!(objects.len(), 0, "dir should only return empty");
+ // List "dir" should return "dir/".
+ let mut obs = op
+ .lister_with(dir.trim_end_matches('/'))
+ .recursive(true)
+ .await?;
+ let mut objects = HashMap::new();
+ while let Some(de) = obs.try_next().await? {
+ objects.insert(de.path().to_string(), de);
+ }
+ assert_eq!(objects.len(), 1, "dir should only return empty");
Review Comment:
ditto
##########
core/tests/behavior/list.rs:
##########
Review Comment:
I think some of the messages could be more specific.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: Add tests for listing recursively on not supported services [incubator-opendal]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #3826:
URL: https://github.com/apache/incubator-opendal/pull/3826#discussion_r1436465896
##########
core/src/types/operator/operator.rs:
##########
@@ -1113,6 +1175,9 @@ impl Operator {
///
/// ## List entries with metakey for more metadata
///
+ /// Some storage services like `s3` could return more metadata like `content-length` and
+ /// `last-modified`. By using `metakey`, we can fetch those metadata without an extra `stat` call.
Review Comment:
I have polished the list_with and lister_with docs, PTAL
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: Add tests for listing recursively on not supported services [incubator-opendal]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #3826:
URL: https://github.com/apache/incubator-opendal/pull/3826#discussion_r1436465263
##########
core/tests/behavior/list.rs:
##########
Review Comment:
That's cool.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org