Re: [PR] [#6131] feat (gvfs-fuse): Add integration test framework of gvfs-fuse [gravitino]
FANNG1 merged PR #6225: URL: https://github.com/apache/gravitino/pull/6225 -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] [#6131] feat (gvfs-fuse): Add integration test framework of gvfs-fuse [gravitino]
FANNG1 closed pull request #6225: [#6131] feat (gvfs-fuse): Add integration test framework of gvfs-fuse URL: https://github.com/apache/gravitino/pull/6225 -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] [#6131] feat (gvfs-fuse): Add integration test framework of gvfs-fuse [gravitino]
FANNG1 merged PR #6160: URL: https://github.com/apache/gravitino/pull/6160 -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] [#6131] feat (gvfs-fuse): Add integration test framework of gvfs-fuse [gravitino]
yuqi1129 commented on PR #6160: URL: https://github.com/apache/gravitino/pull/6160#issuecomment-2589163685 > LGTM, @yuqi1129 any other comments? I have reviewed again and no further comments. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] [#6131] feat (gvfs-fuse): Add integration test framework of gvfs-fuse [gravitino]
yuqi1129 commented on code in PR #6160:
URL: https://github.com/apache/gravitino/pull/6160#discussion_r1914331076
##
.github/workflows/gvfs-fuse-build-test.yml:
##
@@ -71,10 +71,18 @@ jobs:
run: |
dev/ci/check_commands.sh
- - name: Build and test Gravitino
+ - name: Build and test Gvfs-fuse
run: |
./gradlew :clients:filesystem-fuse:build -PenableFuse=true
+ - name: Integration test
+run: |
+ ./gradlew build -x :clients:client-python:build -x test -x web
-PjdkVersion=${{ matrix.java-version }}
+ ./gradlew compileDistribution -x :clients:client-python:build -x
test -x web -PjdkVersion=${{ matrix.java-version }}
Review Comment:
OK
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] [#6131] feat (gvfs-fuse): Add integration test framework of gvfs-fuse [gravitino]
FANNG1 commented on PR #6160: URL: https://github.com/apache/gravitino/pull/6160#issuecomment-2589158524 LGTM, @yuqi1129 any other comments? -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] [#6131] feat (gvfs-fuse): Add integration test framework of gvfs-fuse [gravitino]
diqiu50 commented on code in PR #6160:
URL: https://github.com/apache/gravitino/pull/6160#discussion_r1914176208
##
.github/workflows/gvfs-fuse-build-test.yml:
##
@@ -71,10 +71,18 @@ jobs:
run: |
dev/ci/check_commands.sh
- - name: Build and test Gravitino
+ - name: Build and test Gvfs-fuse
run: |
./gradlew :clients:filesystem-fuse:build -PenableFuse=true
+ - name: Integration test
+run: |
+ ./gradlew build -x :clients:client-python:build -x test -x web
-PjdkVersion=${{ matrix.java-version }}
+ ./gradlew compileDistribution -x :clients:client-python:build -x
test -x web -PjdkVersion=${{ matrix.java-version }}
Review Comment:
The one above builds `gvfs-fuse`, and the below builds the Gravitino
server needed for testing.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] [#6131] feat (gvfs-fuse): Add integration test framework of gvfs-fuse [gravitino]
diqiu50 commented on code in PR #6160: URL: https://github.com/apache/gravitino/pull/6160#discussion_r1914077696 ## .github/workflows/gvfs-fuse-build-test.yml: ## @@ -71,10 +71,18 @@ jobs: run: | dev/ci/check_commands.sh - - name: Build and test Gravitino + - name: Build and test Gvfs-fuse Review Comment: fix -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] [#6131] feat (gvfs-fuse): Add integration test framework of gvfs-fuse [gravitino]
yuqi1129 commented on code in PR #6160:
URL: https://github.com/apache/gravitino/pull/6160#discussion_r1912900961
##
.github/workflows/gvfs-fuse-build-test.yml:
##
@@ -71,10 +71,18 @@ jobs:
run: |
dev/ci/check_commands.sh
- - name: Build and test Gravitino
+ - name: Build and test Gvfs-fuse
run: |
./gradlew :clients:filesystem-fuse:build -PenableFuse=true
+ - name: Integration test
+run: |
+ ./gradlew build -x :clients:client-python:build -x test -x web
-PjdkVersion=${{ matrix.java-version }}
+ ./gradlew compileDistribution -x :clients:client-python:build -x
test -x web -PjdkVersion=${{ matrix.java-version }}
Review Comment:
I believe move this two line to the `Build and test Gvfs-fuse` may be more
proper
##
.github/workflows/gvfs-fuse-build-test.yml:
##
@@ -71,10 +71,18 @@ jobs:
run: |
dev/ci/check_commands.sh
- - name: Build and test Gravitino
+ - name: Build and test Gvfs-fuse
Review Comment:
`Build and test Gvfs-fuse` -> `Build Gvfs-fuse`
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] [#6131] feat (gvfs-fuse): Add integration test framework of gvfs-fuse [gravitino]
diqiu50 commented on code in PR #6160:
URL: https://github.com/apache/gravitino/pull/6160#discussion_r1914030661
##
clients/filesystem-fuse/src/open_dal_filesystem.rs:
##
@@ -261,22 +261,29 @@ fn opendal_filemode_to_filetype(mode: EntryMode) ->
FileType {
mod test {
use crate::config::AppConfig;
use crate::s3_filesystem::extract_s3_config;
+use crate::s3_filesystem::tests::s3_test_config;
+use crate::test_enable_with;
+use crate::RUN_TEST_WITH_S3;
use opendal::layers::LoggingLayer;
use opendal::{services, Builder, Operator};
-#[tokio::test]
-async fn test_s3_stat() {
-let config =
AppConfig::from_file(Some("tests/conf/gvfs_fuse_s3.toml")).unwrap();
-let opendal_config = extract_s3_config(&config);
-
+fn create_opendal(config: &AppConfig) -> Operator {
+let opendal_config = extract_s3_config(config);
let builder = services::S3::from_map(opendal_config);
// Init an operator
-let op = Operator::new(builder)
+Operator::new(builder)
.expect("opendal create failed")
.layer(LoggingLayer::default())
-.finish();
+.finish()
+}
+
+#[tokio::test]
+async fn s3_ut_test_s3_stat() {
+test_enable_with!(RUN_TEST_WITH_S3);
Review Comment:
The storage-s3 feature is used to enable that functionality for all code,
not just for testing.
##
clients/filesystem-fuse/src/open_dal_filesystem.rs:
##
@@ -261,22 +261,29 @@ fn opendal_filemode_to_filetype(mode: EntryMode) ->
FileType {
mod test {
use crate::config::AppConfig;
use crate::s3_filesystem::extract_s3_config;
+use crate::s3_filesystem::tests::s3_test_config;
+use crate::test_enable_with;
+use crate::RUN_TEST_WITH_S3;
use opendal::layers::LoggingLayer;
use opendal::{services, Builder, Operator};
-#[tokio::test]
-async fn test_s3_stat() {
-let config =
AppConfig::from_file(Some("tests/conf/gvfs_fuse_s3.toml")).unwrap();
-let opendal_config = extract_s3_config(&config);
-
+fn create_opendal(config: &AppConfig) -> Operator {
+let opendal_config = extract_s3_config(config);
let builder = services::S3::from_map(opendal_config);
// Init an operator
-let op = Operator::new(builder)
+Operator::new(builder)
.expect("opendal create failed")
.layer(LoggingLayer::default())
-.finish();
+.finish()
+}
+
+#[tokio::test]
+async fn s3_ut_test_s3_stat() {
+test_enable_with!(RUN_TEST_WITH_S3);
Review Comment:
The `storage-s3` feature is used to enable that functionality for all code,
not just for testing.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] [#6131] feat (gvfs-fuse): Add integration test framework of gvfs-fuse [gravitino]
FANNG1 commented on code in PR #6160:
URL: https://github.com/apache/gravitino/pull/6160#discussion_r1914009239
##
clients/filesystem-fuse/src/open_dal_filesystem.rs:
##
@@ -261,22 +261,29 @@ fn opendal_filemode_to_filetype(mode: EntryMode) ->
FileType {
mod test {
use crate::config::AppConfig;
use crate::s3_filesystem::extract_s3_config;
+use crate::s3_filesystem::tests::s3_test_config;
+use crate::test_enable_with;
+use crate::RUN_TEST_WITH_S3;
use opendal::layers::LoggingLayer;
use opendal::{services, Builder, Operator};
-#[tokio::test]
-async fn test_s3_stat() {
-let config =
AppConfig::from_file(Some("tests/conf/gvfs_fuse_s3.toml")).unwrap();
-let opendal_config = extract_s3_config(&config);
-
+fn create_opendal(config: &AppConfig) -> Operator {
+let opendal_config = extract_s3_config(config);
let builder = services::S3::from_map(opendal_config);
// Init an operator
-let op = Operator::new(builder)
+Operator::new(builder)
.expect("opendal create failed")
.layer(LoggingLayer::default())
-.finish();
+.finish()
+}
+
+#[tokio::test]
+async fn s3_ut_test_s3_stat() {
+test_enable_with!(RUN_TEST_WITH_S3);
Review Comment:
please refer to
https://github.com/apache/iceberg-rust/blob/main/crates/iceberg/tests/file_io_s3_test.rs
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] [#6131] feat (gvfs-fuse): Add integration test framework of gvfs-fuse [gravitino]
diqiu50 commented on code in PR #6160:
URL: https://github.com/apache/gravitino/pull/6160#discussion_r1912918206
##
clients/filesystem-fuse/src/open_dal_filesystem.rs:
##
@@ -261,22 +261,29 @@ fn opendal_filemode_to_filetype(mode: EntryMode) ->
FileType {
mod test {
use crate::config::AppConfig;
use crate::s3_filesystem::extract_s3_config;
+use crate::s3_filesystem::tests::s3_test_config;
+use crate::test_enable_with;
+use crate::RUN_TEST_WITH_S3;
use opendal::layers::LoggingLayer;
use opendal::{services, Builder, Operator};
-#[tokio::test]
-async fn test_s3_stat() {
-let config =
AppConfig::from_file(Some("tests/conf/gvfs_fuse_s3.toml")).unwrap();
-let opendal_config = extract_s3_config(&config);
-
+fn create_opendal(config: &AppConfig) -> Operator {
+let opendal_config = extract_s3_config(config);
let builder = services::S3::from_map(opendal_config);
// Init an operator
-let op = Operator::new(builder)
+Operator::new(builder)
.expect("opendal create failed")
.layer(LoggingLayer::default())
-.finish();
+.finish()
+}
+
+#[tokio::test]
+async fn s3_ut_test_s3_stat() {
+test_enable_with!(RUN_TEST_WITH_S3);
Review Comment:
There have a lib can mark tags for the testers. but it can not work with
` #[tokio::test]`
https://docs.rs/tagrs/latest/tagrs/
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] [#6131] feat (gvfs-fuse): Add integration test framework of gvfs-fuse [gravitino]
diqiu50 commented on code in PR #6160:
URL: https://github.com/apache/gravitino/pull/6160#discussion_r1912851198
##
clients/filesystem-fuse/src/open_dal_filesystem.rs:
##
@@ -261,22 +261,29 @@ fn opendal_filemode_to_filetype(mode: EntryMode) ->
FileType {
mod test {
use crate::config::AppConfig;
use crate::s3_filesystem::extract_s3_config;
+use crate::s3_filesystem::tests::s3_test_config;
+use crate::test_enable_with;
+use crate::RUN_TEST_WITH_S3;
use opendal::layers::LoggingLayer;
use opendal::{services, Builder, Operator};
-#[tokio::test]
-async fn test_s3_stat() {
-let config =
AppConfig::from_file(Some("tests/conf/gvfs_fuse_s3.toml")).unwrap();
-let opendal_config = extract_s3_config(&config);
-
+fn create_opendal(config: &AppConfig) -> Operator {
+let opendal_config = extract_s3_config(config);
let builder = services::S3::from_map(opendal_config);
// Init an operator
-let op = Operator::new(builder)
+Operator::new(builder)
.expect("opendal create failed")
.layer(LoggingLayer::default())
-.finish();
+.finish()
+}
+
+#[tokio::test]
+async fn s3_ut_test_s3_stat() {
+test_enable_with!(RUN_TEST_WITH_S3);
Review Comment:
This is generally used to control whether a module participates in
compilation, and I haven't seen any projects using it to control tests. People
familiar with Rust might find this confusing, right?
It also cannot filter out tests that do not have feature = "ENABLE_S3_TEST"
enabled.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] [#6131] feat (gvfs-fuse): Add integration test framework of gvfs-fuse [gravitino]
diqiu50 commented on code in PR #6160:
URL: https://github.com/apache/gravitino/pull/6160#discussion_r1912851198
##
clients/filesystem-fuse/src/open_dal_filesystem.rs:
##
@@ -261,22 +261,29 @@ fn opendal_filemode_to_filetype(mode: EntryMode) ->
FileType {
mod test {
use crate::config::AppConfig;
use crate::s3_filesystem::extract_s3_config;
+use crate::s3_filesystem::tests::s3_test_config;
+use crate::test_enable_with;
+use crate::RUN_TEST_WITH_S3;
use opendal::layers::LoggingLayer;
use opendal::{services, Builder, Operator};
-#[tokio::test]
-async fn test_s3_stat() {
-let config =
AppConfig::from_file(Some("tests/conf/gvfs_fuse_s3.toml")).unwrap();
-let opendal_config = extract_s3_config(&config);
-
+fn create_opendal(config: &AppConfig) -> Operator {
+let opendal_config = extract_s3_config(config);
let builder = services::S3::from_map(opendal_config);
// Init an operator
-let op = Operator::new(builder)
+Operator::new(builder)
.expect("opendal create failed")
.layer(LoggingLayer::default())
-.finish();
+.finish()
+}
+
+#[tokio::test]
+async fn s3_ut_test_s3_stat() {
+test_enable_with!(RUN_TEST_WITH_S3);
Review Comment:
This is generally used to control whether a module participates in
compilation, and I haven't seen any projects using it to control tests. People
familiar with Rust might find this confusing, right?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] [#6131] feat (gvfs-fuse): Add integration test framework of gvfs-fuse [gravitino]
diqiu50 commented on PR #6160: URL: https://github.com/apache/gravitino/pull/6160#issuecomment-2586504518 @yuqi1129 @mchades @xunliu Please take a review. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] [#6131] feat (gvfs-fuse): Add integration test framework of gvfs-fuse [gravitino]
FANNG1 commented on code in PR #6160:
URL: https://github.com/apache/gravitino/pull/6160#discussion_r1912623659
##
clients/filesystem-fuse/src/open_dal_filesystem.rs:
##
@@ -261,22 +261,29 @@ fn opendal_filemode_to_filetype(mode: EntryMode) ->
FileType {
mod test {
use crate::config::AppConfig;
use crate::s3_filesystem::extract_s3_config;
+use crate::s3_filesystem::tests::s3_test_config;
+use crate::test_enable_with;
+use crate::RUN_TEST_WITH_S3;
use opendal::layers::LoggingLayer;
use opendal::{services, Builder, Operator};
-#[tokio::test]
-async fn test_s3_stat() {
-let config =
AppConfig::from_file(Some("tests/conf/gvfs_fuse_s3.toml")).unwrap();
-let opendal_config = extract_s3_config(&config);
-
+fn create_opendal(config: &AppConfig) -> Operator {
+let opendal_config = extract_s3_config(config);
let builder = services::S3::from_map(opendal_config);
// Init an operator
-let op = Operator::new(builder)
+Operator::new(builder)
.expect("opendal create failed")
.layer(LoggingLayer::default())
-.finish();
+.finish()
+}
+
+#[tokio::test]
+async fn s3_ut_test_s3_stat() {
+test_enable_with!(RUN_TEST_WITH_S3);
Review Comment:
try using this?
```#[cfg(feature = "ENABLE_S3_TEST")] ```, after using this macro, seems
we could remove `s3_ut` prefix too.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] [#6131] feat (gvfs-fuse): Add integration test framework of gvfs-fuse [gravitino]
FANNG1 commented on code in PR #6160:
URL: https://github.com/apache/gravitino/pull/6160#discussion_r1912623659
##
clients/filesystem-fuse/src/open_dal_filesystem.rs:
##
@@ -261,22 +261,29 @@ fn opendal_filemode_to_filetype(mode: EntryMode) ->
FileType {
mod test {
use crate::config::AppConfig;
use crate::s3_filesystem::extract_s3_config;
+use crate::s3_filesystem::tests::s3_test_config;
+use crate::test_enable_with;
+use crate::RUN_TEST_WITH_S3;
use opendal::layers::LoggingLayer;
use opendal::{services, Builder, Operator};
-#[tokio::test]
-async fn test_s3_stat() {
-let config =
AppConfig::from_file(Some("tests/conf/gvfs_fuse_s3.toml")).unwrap();
-let opendal_config = extract_s3_config(&config);
-
+fn create_opendal(config: &AppConfig) -> Operator {
+let opendal_config = extract_s3_config(config);
let builder = services::S3::from_map(opendal_config);
// Init an operator
-let op = Operator::new(builder)
+Operator::new(builder)
.expect("opendal create failed")
.layer(LoggingLayer::default())
-.finish();
+.finish()
+}
+
+#[tokio::test]
+async fn s3_ut_test_s3_stat() {
+test_enable_with!(RUN_TEST_WITH_S3);
Review Comment:
try using this?
```#[cfg(feature = "ENABLE_S3_TEST")] ```
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] [#6131] feat (gvfs-fuse): Add integration test framework of gvfs-fuse [gravitino]
diqiu50 commented on code in PR #6160:
URL: https://github.com/apache/gravitino/pull/6160#discussion_r1910096413
##
clients/filesystem-fuse/tests/fuse_test.rs:
##
@@ -106,14 +107,21 @@ fn test_fuse_system_with_auto() {
test_fuse_filesystem(mount_point);
}
-fn test_fuse_system_with_manual() {
-test_fuse_filesystem("build/gvfs");
+#[test]
+fn fuse_it_test_fuse() {
+test_enable_with!(RUN_TEST_WITH_FUSE);
+
+test_fuse_filesystem("target/gvfs/gvfs_test");
}
fn test_fuse_filesystem(mount_point: &str) {
Review Comment:
add script to start up other test environment and and arguments in
`run_fuse_testers.sh` to select which backend to run
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] [#6131] feat (gvfs-fuse): Add integration test framework of gvfs-fuse [gravitino]
diqiu50 commented on code in PR #6160:
URL: https://github.com/apache/gravitino/pull/6160#discussion_r1910096413
##
clients/filesystem-fuse/tests/fuse_test.rs:
##
@@ -106,14 +107,21 @@ fn test_fuse_system_with_auto() {
test_fuse_filesystem(mount_point);
}
-fn test_fuse_system_with_manual() {
-test_fuse_filesystem("build/gvfs");
+#[test]
+fn fuse_it_test_fuse() {
+test_enable_with!(RUN_TEST_WITH_FUSE);
+
+test_fuse_filesystem("target/gvfs/gvfs_test");
}
fn test_fuse_filesystem(mount_point: &str) {
Review Comment:
add script to start up other test environment and and arguments in
run_fuse_testers.sh to select with backend to run
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] [#6131] feat (gvfs-fuse): Add integration test framework of gvfs-fuse [gravitino]
diqiu50 commented on code in PR #6160:
URL: https://github.com/apache/gravitino/pull/6160#discussion_r1910096413
##
clients/filesystem-fuse/tests/fuse_test.rs:
##
@@ -106,14 +107,21 @@ fn test_fuse_system_with_auto() {
test_fuse_filesystem(mount_point);
}
-fn test_fuse_system_with_manual() {
-test_fuse_filesystem("build/gvfs");
+#[test]
+fn fuse_it_test_fuse() {
+test_enable_with!(RUN_TEST_WITH_FUSE);
+
+test_fuse_filesystem("target/gvfs/gvfs_test");
}
fn test_fuse_filesystem(mount_point: &str) {
Review Comment:
add script or arguments of `tests/bin/xxx.sh` to start up other test
environment like run_fuse_testers.sh
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] [#6131] feat (gvfs-fuse): Add integration test framework of gvfs-fuse [gravitino]
FANNG1 commented on code in PR #6160:
URL: https://github.com/apache/gravitino/pull/6160#discussion_r1910079089
##
clients/filesystem-fuse/tests/fuse_test.rs:
##
@@ -106,14 +107,21 @@ fn test_fuse_system_with_auto() {
test_fuse_filesystem(mount_point);
}
-fn test_fuse_system_with_manual() {
-test_fuse_filesystem("build/gvfs");
+#[test]
+fn fuse_it_test_fuse() {
+test_enable_with!(RUN_TEST_WITH_FUSE);
+
+test_fuse_filesystem("target/gvfs/gvfs_test");
}
fn test_fuse_filesystem(mount_point: &str) {
Review Comment:
I know, How do you plan to support other storages in this test?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] [#6131] feat (gvfs-fuse): Add integration test framework of gvfs-fuse [gravitino]
FANNG1 commented on code in PR #6160:
URL: https://github.com/apache/gravitino/pull/6160#discussion_r1910079089
##
clients/filesystem-fuse/tests/fuse_test.rs:
##
@@ -106,14 +107,21 @@ fn test_fuse_system_with_auto() {
test_fuse_filesystem(mount_point);
}
-fn test_fuse_system_with_manual() {
-test_fuse_filesystem("build/gvfs");
+#[test]
+fn fuse_it_test_fuse() {
+test_enable_with!(RUN_TEST_WITH_FUSE);
+
+test_fuse_filesystem("target/gvfs/gvfs_test");
}
fn test_fuse_filesystem(mount_point: &str) {
Review Comment:
I know, How do you plan to support other storages?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] [#6131] feat (gvfs-fuse): Add integration test framework of gvfs-fuse [gravitino]
diqiu50 commented on code in PR #6160:
URL: https://github.com/apache/gravitino/pull/6160#discussion_r1910078735
##
clients/filesystem-fuse/tests/fuse_test.rs:
##
@@ -106,14 +107,21 @@ fn test_fuse_system_with_auto() {
test_fuse_filesystem(mount_point);
}
-fn test_fuse_system_with_manual() {
-test_fuse_filesystem("build/gvfs");
+#[test]
+fn fuse_it_test_fuse() {
+test_enable_with!(RUN_TEST_WITH_FUSE);
+
+test_fuse_filesystem("target/gvfs/gvfs_test");
}
fn test_fuse_filesystem(mount_point: &str) {
Review Comment:
Integration tests generally do not care about what the underlying file
system actually is.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] [#6131] feat (gvfs-fuse): Add integration test framework of gvfs-fuse [gravitino]
diqiu50 commented on code in PR #6160:
URL: https://github.com/apache/gravitino/pull/6160#discussion_r1910074917
##
clients/filesystem-fuse/tests/fuse_test.rs:
##
@@ -106,14 +107,21 @@ fn test_fuse_system_with_auto() {
test_fuse_filesystem(mount_point);
}
-fn test_fuse_system_with_manual() {
-test_fuse_filesystem("build/gvfs");
+#[test]
+fn fuse_it_test_fuse() {
+test_enable_with!(RUN_TEST_WITH_FUSE);
+
+test_fuse_filesystem("target/gvfs/gvfs_test");
}
fn test_fuse_filesystem(mount_point: &str) {
Review Comment:
This test is used to verify the directory after the FUSE filesystem is
mounted. It doesn’t matter whether the underlying storage is S3, GCS, or
anything else.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] [#6131] feat (gvfs-fuse): Add integration test framework of gvfs-fuse [gravitino]
FANNG1 commented on code in PR #6160:
URL: https://github.com/apache/gravitino/pull/6160#discussion_r1910065538
##
clients/filesystem-fuse/tests/fuse_test.rs:
##
@@ -106,14 +107,21 @@ fn test_fuse_system_with_auto() {
test_fuse_filesystem(mount_point);
}
-fn test_fuse_system_with_manual() {
-test_fuse_filesystem("build/gvfs");
+#[test]
+fn fuse_it_test_fuse() {
+test_enable_with!(RUN_TEST_WITH_FUSE);
+
+test_fuse_filesystem("target/gvfs/gvfs_test");
}
fn test_fuse_filesystem(mount_point: &str) {
Review Comment:
does this test s3 filesystem? how do you plan to support other cloud system
like GCS? I think you need an abstraction.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] [#6131] feat (gvfs-fuse): Add integration test framework of gvfs-fuse [gravitino]
diqiu50 commented on code in PR #6160:
URL: https://github.com/apache/gravitino/pull/6160#discussion_r1910059867
##
clients/filesystem-fuse/src/open_dal_filesystem.rs:
##
@@ -261,22 +261,29 @@ fn opendal_filemode_to_filetype(mode: EntryMode) ->
FileType {
mod test {
use crate::config::AppConfig;
use crate::s3_filesystem::extract_s3_config;
+use crate::s3_filesystem::tests::s3_test_config;
+use crate::test_enable_with;
+use crate::RUN_TEST_WITH_S3;
use opendal::layers::LoggingLayer;
use opendal::{services, Builder, Operator};
-#[tokio::test]
-async fn test_s3_stat() {
-let config =
AppConfig::from_file(Some("tests/conf/gvfs_fuse_s3.toml")).unwrap();
-let opendal_config = extract_s3_config(&config);
-
+fn create_opendal(config: &AppConfig) -> Operator {
+let opendal_config = extract_s3_config(config);
let builder = services::S3::from_map(opendal_config);
// Init an operator
-let op = Operator::new(builder)
+Operator::new(builder)
.expect("opendal create failed")
.layer(LoggingLayer::default())
-.finish();
+.finish()
+}
+
+#[tokio::test]
+async fn s3_ut_test_s3_stat() {
+test_enable_with!(RUN_TEST_WITH_S3);
Review Comment:
I can’t find a good way to mark the test tags and run them.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] [#6131] feat (gvfs-fuse): Add integration test framework of gvfs-fuse [gravitino]
diqiu50 commented on code in PR #6160:
URL: https://github.com/apache/gravitino/pull/6160#discussion_r1910056200
##
clients/filesystem-fuse/src/open_dal_filesystem.rs:
##
@@ -261,22 +261,29 @@ fn opendal_filemode_to_filetype(mode: EntryMode) ->
FileType {
mod test {
use crate::config::AppConfig;
use crate::s3_filesystem::extract_s3_config;
+use crate::s3_filesystem::tests::s3_test_config;
+use crate::test_enable_with;
+use crate::RUN_TEST_WITH_S3;
use opendal::layers::LoggingLayer;
use opendal::{services, Builder, Operator};
-#[tokio::test]
-async fn test_s3_stat() {
-let config =
AppConfig::from_file(Some("tests/conf/gvfs_fuse_s3.toml")).unwrap();
-let opendal_config = extract_s3_config(&config);
-
+fn create_opendal(config: &AppConfig) -> Operator {
+let opendal_config = extract_s3_config(config);
let builder = services::S3::from_map(opendal_config);
// Init an operator
-let op = Operator::new(builder)
+Operator::new(builder)
.expect("opendal create failed")
.layer(LoggingLayer::default())
-.finish();
+.finish()
+}
+
+#[tokio::test]
+async fn s3_ut_test_s3_stat() {
Review Comment:
`s3_ut_` is a special name used to select tests that need environment
variable of `RUN_TEST_WITH_S3`
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] [#6131] feat (gvfs-fuse): Add integration test framework of gvfs-fuse [gravitino]
FANNG1 commented on code in PR #6160:
URL: https://github.com/apache/gravitino/pull/6160#discussion_r1910047540
##
clients/filesystem-fuse/src/open_dal_filesystem.rs:
##
@@ -261,22 +261,29 @@ fn opendal_filemode_to_filetype(mode: EntryMode) ->
FileType {
mod test {
use crate::config::AppConfig;
use crate::s3_filesystem::extract_s3_config;
+use crate::s3_filesystem::tests::s3_test_config;
+use crate::test_enable_with;
+use crate::RUN_TEST_WITH_S3;
use opendal::layers::LoggingLayer;
use opendal::{services, Builder, Operator};
-#[tokio::test]
-async fn test_s3_stat() {
-let config =
AppConfig::from_file(Some("tests/conf/gvfs_fuse_s3.toml")).unwrap();
-let opendal_config = extract_s3_config(&config);
-
+fn create_opendal(config: &AppConfig) -> Operator {
+let opendal_config = extract_s3_config(config);
let builder = services::S3::from_map(opendal_config);
// Init an operator
-let op = Operator::new(builder)
+Operator::new(builder)
.expect("opendal create failed")
.layer(LoggingLayer::default())
-.finish();
+.finish()
+}
+
+#[tokio::test]
+async fn s3_ut_test_s3_stat() {
Review Comment:
`s3_ut_test_s3_stat` is duplicate, How about `test_s3_stat`?
##
clients/filesystem-fuse/src/open_dal_filesystem.rs:
##
@@ -261,22 +261,29 @@ fn opendal_filemode_to_filetype(mode: EntryMode) ->
FileType {
mod test {
use crate::config::AppConfig;
use crate::s3_filesystem::extract_s3_config;
+use crate::s3_filesystem::tests::s3_test_config;
+use crate::test_enable_with;
+use crate::RUN_TEST_WITH_S3;
use opendal::layers::LoggingLayer;
use opendal::{services, Builder, Operator};
-#[tokio::test]
-async fn test_s3_stat() {
-let config =
AppConfig::from_file(Some("tests/conf/gvfs_fuse_s3.toml")).unwrap();
-let opendal_config = extract_s3_config(&config);
-
+fn create_opendal(config: &AppConfig) -> Operator {
+let opendal_config = extract_s3_config(config);
let builder = services::S3::from_map(opendal_config);
// Init an operator
-let op = Operator::new(builder)
+Operator::new(builder)
.expect("opendal create failed")
.layer(LoggingLayer::default())
-.finish();
+.finish()
+}
+
+#[tokio::test]
+async fn s3_ut_test_s3_stat() {
+test_enable_with!(RUN_TEST_WITH_S3);
Review Comment:
this seems hacks, could you refer other rust projects for more general way
to control the tests?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] [#6131] feat (gvfs-fuse): Add integration test framework of gvfs-fuse [gravitino]
diqiu50 commented on code in PR #6160:
URL: https://github.com/apache/gravitino/pull/6160#discussion_r1910049284
##
clients/filesystem-fuse/src/gravitino_fileset_filesystem.rs:
##
@@ -172,4 +183,68 @@ mod tests {
let path =
fs.raw_path_to_gvfs_path(Path::new("/c1/fileset1")).unwrap();
assert_eq!(path, Path::new("/"));
}
+
+async fn create_fileset_fs(path: &Path, config: &AppConfig) ->
GravitinoFilesetFileSystem {
+let opendal_config = extract_s3_config(config);
+
+cleanup_s3_fs(path, &opendal_config).await;
+
+let bucket = opendal_config.get("bucket").expect("Bucket must exist");
+let endpoint = opendal_config.get("endpoint").expect("Endpoint must
exist");
+
+let catalog = create_test_catalog(
+"c1",
+"s3",
+vec![
+("location".to_string(), format!("s3a://{}", bucket)),
+("s3-endpoint".to_string(), endpoint.to_string()),
+]
+.into_iter()
+.collect::>(),
+);
+let file_set_location = format!("s3a://{}{}", bucket,
path.to_string_lossy());
+let file_set = create_test_fileset("fileset1", &file_set_location);
+
+let fs_context = FileSystemContext::default();
+let inner_fs = create_fs_with_fileset(&catalog, &file_set, config,
&fs_context)
+.await
+.unwrap();
+GravitinoFilesetFileSystem::new(
+inner_fs,
+path,
+GravitinoClient::new(&config.gravitino),
+config,
+&fs_context,
+)
+.await
+}
+
+#[tokio::test]
+async fn s3_ut_test_fileset_file_system() {
Review Comment:
All ITs in the tests directory
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] [#6131] feat (gvfs-fuse): Add integration test framework of gvfs-fuse [gravitino]
diqiu50 commented on code in PR #6160:
URL: https://github.com/apache/gravitino/pull/6160#discussion_r1910047829
##
clients/filesystem-fuse/src/gravitino_fileset_filesystem.rs:
##
@@ -172,4 +183,68 @@ mod tests {
let path =
fs.raw_path_to_gvfs_path(Path::new("/c1/fileset1")).unwrap();
assert_eq!(path, Path::new("/"));
}
+
+async fn create_fileset_fs(path: &Path, config: &AppConfig) ->
GravitinoFilesetFileSystem {
+let opendal_config = extract_s3_config(config);
+
+cleanup_s3_fs(path, &opendal_config).await;
+
+let bucket = opendal_config.get("bucket").expect("Bucket must exist");
+let endpoint = opendal_config.get("endpoint").expect("Endpoint must
exist");
+
+let catalog = create_test_catalog(
+"c1",
+"s3",
+vec![
+("location".to_string(), format!("s3a://{}", bucket)),
+("s3-endpoint".to_string(), endpoint.to_string()),
+]
+.into_iter()
+.collect::>(),
+);
+let file_set_location = format!("s3a://{}{}", bucket,
path.to_string_lossy());
+let file_set = create_test_fileset("fileset1", &file_set_location);
+
+let fs_context = FileSystemContext::default();
+let inner_fs = create_fs_with_fileset(&catalog, &file_set, config,
&fs_context)
+.await
+.unwrap();
+GravitinoFilesetFileSystem::new(
+inner_fs,
+path,
+GravitinoClient::new(&config.gravitino),
+config,
+&fs_context,
+)
+.await
+}
+
+#[tokio::test]
+async fn s3_ut_test_fileset_file_system() {
Review Comment:
This is ut.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] [#6131] feat (gvfs-fuse): Add integration test framework of gvfs-fuse [gravitino]
FANNG1 commented on code in PR #6160:
URL: https://github.com/apache/gravitino/pull/6160#discussion_r1910045102
##
clients/filesystem-fuse/src/gravitino_fileset_filesystem.rs:
##
@@ -172,4 +183,68 @@ mod tests {
let path =
fs.raw_path_to_gvfs_path(Path::new("/c1/fileset1")).unwrap();
assert_eq!(path, Path::new("/"));
}
+
+async fn create_fileset_fs(path: &Path, config: &AppConfig) ->
GravitinoFilesetFileSystem {
+let opendal_config = extract_s3_config(config);
+
+cleanup_s3_fs(path, &opendal_config).await;
+
+let bucket = opendal_config.get("bucket").expect("Bucket must exist");
+let endpoint = opendal_config.get("endpoint").expect("Endpoint must
exist");
+
+let catalog = create_test_catalog(
+"c1",
+"s3",
+vec![
+("location".to_string(), format!("s3a://{}", bucket)),
+("s3-endpoint".to_string(), endpoint.to_string()),
+]
+.into_iter()
+.collect::>(),
+);
+let file_set_location = format!("s3a://{}{}", bucket,
path.to_string_lossy());
+let file_set = create_test_fileset("fileset1", &file_set_location);
+
+let fs_context = FileSystemContext::default();
+let inner_fs = create_fs_with_fileset(&catalog, &file_set, config,
&fs_context)
+.await
+.unwrap();
+GravitinoFilesetFileSystem::new(
+inner_fs,
+path,
+GravitinoClient::new(&config.gravitino),
+config,
+&fs_context,
+)
+.await
+}
+
+#[tokio::test]
+async fn s3_ut_test_fileset_file_system() {
Review Comment:
this seems an IT not UT? and could you place IT in separate files?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] [#6131] feat (gvfs-fuse): Add integration test framework of gvfs-fuse [gravitino]
diqiu50 commented on code in PR #6160:
URL: https://github.com/apache/gravitino/pull/6160#discussion_r1910019455
##
clients/filesystem-fuse/src/default_raw_filesystem.rs:
##
@@ -334,13 +334,21 @@ impl RawFileSystem for
DefaultRawFileSystem {
file.flush().await
}
-async fn close_file(&self, _file_id: u64, fh: u64) -> Result<()> {
+async fn close_file(&self, file_id: u64, fh: u64) -> Result<()> {
+let file_entry = self.get_file_entry(file_id).await;
Review Comment:
Yes, closing deleted file may cause bugs.
This fix is not good. Add the todo comments to handle that case.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] [#6131] feat (gvfs-fuse): Add integration test framework of gvfs-fuse [gravitino]
diqiu50 commented on code in PR #6160:
URL: https://github.com/apache/gravitino/pull/6160#discussion_r1910016768
##
.github/workflows/gvfs-fuse-build-test.yml:
##
@@ -71,10 +71,17 @@ jobs:
run: |
dev/ci/check_commands.sh
- - name: Build and test Gravitino
+ - name: Build and test Gvfs-fuse
run: |
./gradlew :clients:filesystem-fuse:build -PenableFuse=true
+ - name: Integration test
+run: |
+ ./gradlew bundles:aws-bundle:build -x :clients:client-python:build
compileDistribution -x test -x web -PjdkVersion=${{ matrix.java-version }}
Review Comment:
fix
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] [#6131] feat (gvfs-fuse): Add integration test framework of gvfs-fuse [gravitino]
FANNG1 commented on code in PR #6160:
URL: https://github.com/apache/gravitino/pull/6160#discussion_r1909920648
##
clients/filesystem-fuse/src/default_raw_filesystem.rs:
##
@@ -334,13 +334,21 @@ impl RawFileSystem for
DefaultRawFileSystem {
file.flush().await
}
-async fn close_file(&self, _file_id: u64, fh: u64) -> Result<()> {
+async fn close_file(&self, file_id: u64, fh: u64) -> Result<()> {
+let file_entry = self.get_file_entry(file_id).await;
Review Comment:
If there is no file entry, is it expected not closing the open file?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] [#6131] feat (gvfs-fuse): Add integration test framework of gvfs-fuse [gravitino]
FANNG1 commented on code in PR #6160:
URL: https://github.com/apache/gravitino/pull/6160#discussion_r1909917616
##
.github/workflows/gvfs-fuse-build-test.yml:
##
@@ -71,10 +71,17 @@ jobs:
run: |
dev/ci/check_commands.sh
- - name: Build and test Gravitino
+ - name: Build and test Gvfs-fuse
run: |
./gradlew :clients:filesystem-fuse:build -PenableFuse=true
+ - name: Integration test
+run: |
+ ./gradlew bundles:aws-bundle:build -x :clients:client-python:build
compileDistribution -x test -x web -PjdkVersion=${{ matrix.java-version }}
Review Comment:
yes, the current IT doesn't automatically build the package, but here you
depends on `bundles:aws-bundle:build`, I wonder whether you could make it auto
dependents. and There are separate actions in Gravitino IT, seems better
make rust client keep consistent with it.
```
- name: Package Gravitino
if: ${{ inputs.test-mode == 'deploy' }}
run: |
./gradlew compileDistribution -x test -PjdkVersion=${{
inputs.java-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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] [#6131] feat (gvfs-fuse): Add integration test framework of gvfs-fuse [gravitino]
diqiu50 commented on code in PR #6160: URL: https://github.com/apache/gravitino/pull/6160#discussion_r1909697407 ## clients/filesystem-fuse/Makefile: ## @@ -62,6 +62,12 @@ doc-test: unit-test: doc-test cargo test --no-fail-fast --lib --all-features --workspace +test-fuse: + @bash ./tests/bin/s3_fileset_it.sh test + +test-s3: Review Comment: fix -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] [#6131] feat (gvfs-fuse): Add integration test framework of gvfs-fuse [gravitino]
diqiu50 commented on code in PR #6160:
URL: https://github.com/apache/gravitino/pull/6160#discussion_r1909675451
##
clients/filesystem-fuse/src/gravitino_client.rs:
##
@@ -35,6 +35,18 @@ pub(crate) struct Fileset {
properties: HashMap,
}
+impl Fileset {
+pub fn new(name: &str, storage_location: &str) -> Fileset {
+Self {
+name: name.to_string(),
+fileset_type: "managed".to_string(),
Review Comment:
move this function to test.
##
clients/filesystem-fuse/src/gravitino_client.rs:
##
@@ -58,6 +70,18 @@ pub(crate) struct Catalog {
pub(crate) properties: HashMap,
}
+impl Catalog {
+pub fn new(name: &str, properties: HashMap) -> Catalog {
Review Comment:
move this function to test.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] [#6131] feat (gvfs-fuse): Add integration test framework of gvfs-fuse [gravitino]
diqiu50 commented on code in PR #6160:
URL: https://github.com/apache/gravitino/pull/6160#discussion_r1909675451
##
clients/filesystem-fuse/src/gravitino_client.rs:
##
@@ -35,6 +35,18 @@ pub(crate) struct Fileset {
properties: HashMap,
}
+impl Fileset {
+pub fn new(name: &str, storage_location: &str) -> Fileset {
+Self {
+name: name.to_string(),
+fileset_type: "managed".to_string(),
Review Comment:
move these function to test.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] [#6131] feat (gvfs-fuse): Add integration test framework of gvfs-fuse [gravitino]
diqiu50 commented on code in PR #6160:
URL: https://github.com/apache/gravitino/pull/6160#discussion_r1909675612
##
clients/filesystem-fuse/src/gravitino_client.rs:
##
@@ -58,6 +70,18 @@ pub(crate) struct Catalog {
pub(crate) properties: HashMap,
}
+impl Catalog {
+pub fn new(name: &str, properties: HashMap) -> Catalog {
Review Comment:
move these function to test.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] [#6131] feat (gvfs-fuse): Add integration test framework of gvfs-fuse [gravitino]
diqiu50 commented on code in PR #6160:
URL: https://github.com/apache/gravitino/pull/6160#discussion_r1909673189
##
clients/filesystem-fuse/src/default_raw_filesystem.rs:
##
@@ -334,13 +334,21 @@ impl RawFileSystem for
DefaultRawFileSystem {
file.flush().await
}
-async fn close_file(&self, _file_id: u64, fh: u64) -> Result<()> {
+async fn close_file(&self, file_id: u64, fh: u64) -> Result<()> {
+let file_entry = self.get_file_entry(file_id).await;
Review Comment:
Check if the file has been deleted.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] [#6131] feat (gvfs-fuse): Add integration test framework of gvfs-fuse [gravitino]
diqiu50 commented on code in PR #6160:
URL: https://github.com/apache/gravitino/pull/6160#discussion_r1909672022
##
.github/workflows/gvfs-fuse-build-test.yml:
##
@@ -71,10 +71,17 @@ jobs:
run: |
dev/ci/check_commands.sh
- - name: Build and test Gravitino
+ - name: Build and test Gvfs-fuse
run: |
./gradlew :clients:filesystem-fuse:build -PenableFuse=true
+ - name: Integration test
+run: |
+ ./gradlew bundles:aws-bundle:build -x :clients:client-python:build
compileDistribution -x test -x web -PjdkVersion=${{ matrix.java-version }}
Review Comment:
It's not good to let make call gradle again.
Currently, the integration test module does not automatically build the
package, right?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] [#6131] feat (gvfs-fuse): Add integration test framework of gvfs-fuse [gravitino]
FANNG1 commented on code in PR #6160:
URL: https://github.com/apache/gravitino/pull/6160#discussion_r1908845828
##
.github/workflows/gvfs-fuse-build-test.yml:
##
@@ -71,10 +71,17 @@ jobs:
run: |
dev/ci/check_commands.sh
- - name: Build and test Gravitino
+ - name: Build and test Gvfs-fuse
run: |
./gradlew :clients:filesystem-fuse:build -PenableFuse=true
+ - name: Integration test
+run: |
+ ./gradlew bundles:aws-bundle:build -x :clients:client-python:build
compileDistribution -x test -x web -PjdkVersion=${{ matrix.java-version }}
Review Comment:
it's hard to use, use `make prepare-it-env` ?
##
clients/filesystem-fuse/src/gravitino_client.rs:
##
@@ -35,6 +35,18 @@ pub(crate) struct Fileset {
properties: HashMap,
}
+impl Fileset {
+pub fn new(name: &str, storage_location: &str) -> Fileset {
+Self {
+name: name.to_string(),
+fileset_type: "managed".to_string(),
Review Comment:
do we support non managed fileset?
##
clients/filesystem-fuse/Makefile:
##
@@ -62,6 +62,12 @@ doc-test:
unit-test: doc-test
cargo test --no-fail-fast --lib --all-features --workspace
+test-fuse:
+ @bash ./tests/bin/s3_fileset_it.sh test
+
+test-s3:
Review Comment:
it's hard to understand the test from the name, use better name?
##
clients/filesystem-fuse/src/gravitino_client.rs:
##
@@ -58,6 +70,18 @@ pub(crate) struct Catalog {
pub(crate) properties: HashMap,
}
+impl Catalog {
+pub fn new(name: &str, properties: HashMap) -> Catalog {
Review Comment:
rename to `new_fileset_with_s3` or move `catalog_type`, `provider` to the
parameters?
##
clients/filesystem-fuse/src/default_raw_filesystem.rs:
##
@@ -334,13 +334,21 @@ impl RawFileSystem for
DefaultRawFileSystem {
file.flush().await
}
-async fn close_file(&self, _file_id: u64, fh: u64) -> Result<()> {
+async fn close_file(&self, file_id: u64, fh: u64) -> Result<()> {
+let file_entry = self.get_file_entry(file_id).await;
Review Comment:
why adding this?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
