You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2021/12/21 12:24:51 UTC
[GitHub] [arrow-datafusion] alamb opened a new pull request #1471: Add section on testing to the Developers doc
alamb opened a new pull request #1471:
URL: https://github.com/apache/arrow-datafusion/pull/1471
# Which issue does this PR close?
re https://github.com/apache/arrow-datafusion/issues/743 and https://github.com/apache/arrow-datafusion/issues/1453
# Rationale for this change
Start to document the current (and maybe aspirational) test structure so we know what, if anything, should be changed as we "clean things up"
# What changes are included in this PR?
A section on `Testing` in the DEVELOPERS doc
# Are there any user-facing changes?
no
cc @hntd187 @xudong963 @Jimexist @houqp
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] hntd187 commented on a change in pull request #1471: Add section on testing to the Developers doc
Posted by GitBox <gi...@apache.org>.
hntd187 commented on a change in pull request #1471:
URL: https://github.com/apache/arrow-datafusion/pull/1471#discussion_r774641595
##########
File path: DEVELOPERS.md
##########
@@ -40,6 +40,57 @@ Testing setup:
- `export PARQUET_TEST_DATA=$(pwd)/parquet-testing/data/`
- `export ARROW_TEST_DATA=$(pwd)/testing/data/`
+
+## Test Organization
+
+DataFusion has several levels of tests in its [Test
+Pyramid](https://martinfowler.com/articles/practical-test-pyramid.html)
+and tries to follow [Testing Organization](https://doc.rust-lang.org/book/ch11-03-test-organization.html) in the The Book.
+
+This section highlights the most important test modules that exist
+
+### Unit tests
+
+Tests for the code in an individual module are defined in the same source file with a `test` module, following Rust convention
+
+For example, the logic for pruning predicates is defined in [pruning.rs](https://github.com/apache/arrow-datafusion/blob/main/datafusion/src/physical_optimizer/pruning.rs#L708) using the
+
+```rust
+#[cfg(test)]
+mod tests {
+...
+}
+```
+
+
+### Rust Integration Tests
+
+There are several tests of the public interface of the DataFusion library in the [tests](https://github.com/apache/arrow-datafusion/blob/master/datafusion/tests) directory.
+
+You can run these tests individually using a command such as
+
+```shell
+cargo test -p datafusion --tests sql
+```
+
+The tests are:
Review comment:
Sure I've started moving things around, and I guess I'm wondering how deep down the rabbit hole do we wanna go here? There are a good number of tests in `sql.rs` that by their name cross considerations, like is reading parquet as a format something for sql? In addition a lot of tests are prepended with csv, which seems somewhat redundant to me, is there a purpose behind it? I think I will reorganize the tests themselves first, touch no code other than moving around and then we can talk about renaming or any actual code changes.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] matthewmturner commented on a change in pull request #1471: Add section on testing to the Developers doc
Posted by GitBox <gi...@apache.org>.
matthewmturner commented on a change in pull request #1471:
URL: https://github.com/apache/arrow-datafusion/pull/1471#discussion_r773529903
##########
File path: DEVELOPERS.md
##########
@@ -40,6 +40,57 @@ Testing setup:
- `export PARQUET_TEST_DATA=$(pwd)/parquet-testing/data/`
- `export ARROW_TEST_DATA=$(pwd)/testing/data/`
+
+## Test Organization
+
+DataFusion has several levels of tests in its [Test
+Pyramid](https://martinfowler.com/articles/practical-test-pyramid.html)
+and tries to follow [Testing Organization](https://doc.rust-lang.org/book/ch11-03-test-organization.html) in the The Book.
+
+This section highlights the most important test modules that exist
+
+### Unit tests
+
+Tests for the code in an individual module are defined in the same source file with a `test` module, following Rust convention
+
+For example, the logic for pruning predicates is defined in [pruning.rs](https://github.com/apache/arrow-datafusion/blob/main/datafusion/src/physical_optimizer/pruning.rs#L708) using the
+
+```rust
+#[cfg(test)]
+mod tests {
+...
+}
+```
+
+
+### Rust Integration Tests
+
+There are several tests of the public interface of the DataFusion library in the [tests](https://github.com/apache/arrow-datafusion/blob/master/datafusion/tests) directory.
+
+You can run these tests individually using a command such as
+
+```shell
+cargo test -p datafusion --tests sql
+```
+
+The tests are:
Review comment:
@alamb @hntd187 sounds good. i have not had a chance to start on this yet and likely wont be able to spend much time on it until i finish some other PRs. @hntd187 it seems like you have a good plan and id be happy to follow your lead. assuming that breaking out the sql functionality like that is the agreed upon approach maybe we could make a parent issue with sub tasks for each functionality / internal thing and then i could pick up on that once i have some more time?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb merged pull request #1471: Add section on testing to the Developers doc
Posted by GitBox <gi...@apache.org>.
alamb merged pull request #1471:
URL: https://github.com/apache/arrow-datafusion/pull/1471
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb commented on a change in pull request #1471: Add section on testing to the Developers doc
Posted by GitBox <gi...@apache.org>.
alamb commented on a change in pull request #1471:
URL: https://github.com/apache/arrow-datafusion/pull/1471#discussion_r773093645
##########
File path: DEVELOPERS.md
##########
@@ -40,6 +40,57 @@ Testing setup:
- `export PARQUET_TEST_DATA=$(pwd)/parquet-testing/data/`
- `export ARROW_TEST_DATA=$(pwd)/testing/data/`
+
+## Test Organization
+
+DataFusion has several levels of tests in its [Test
+Pyramid](https://martinfowler.com/articles/practical-test-pyramid.html)
+and tries to follow [Testing Organization](https://doc.rust-lang.org/book/ch11-03-test-organization.html) in the The Book.
+
+This section highlights the most important test modules that exist
+
+### Unit tests
+
+Tests for the code in an individual module are defined in the same source file with a `test` module, following Rust convention
+
+For example, the logic for pruning predicates is defined in [pruning.rs](https://github.com/apache/arrow-datafusion/blob/main/datafusion/src/physical_optimizer/pruning.rs#L708) using the
+
+```rust
+#[cfg(test)]
+mod tests {
+...
+}
+```
+
+
+### Rust Integration Tests
+
+There are several tests of the public interface of the DataFusion library in the [tests](https://github.com/apache/arrow-datafusion/blob/master/datafusion/tests) directory.
+
+You can run these tests individually using a command such as
+
+```shell
+cargo test -p datafusion --tests sql
+```
+
+The tests are:
Review comment:
One thing I was thinking @hntd187 and @matthewmturner would be to put some guidance here like "tests that run a full sql query likely belong in `sql.rs`" (also perhaps describe which tests should go in the "SQL / Postgres integration tests" and which are good to be in sql.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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb commented on a change in pull request #1471: Add section on testing to the Developers doc
Posted by GitBox <gi...@apache.org>.
alamb commented on a change in pull request #1471:
URL: https://github.com/apache/arrow-datafusion/pull/1471#discussion_r804203912
##########
File path: DEVELOPERS.md
##########
@@ -40,6 +40,80 @@ Testing setup:
- `export PARQUET_TEST_DATA=$(pwd)/parquet-testing/data/`
- `export ARROW_TEST_DATA=$(pwd)/testing/data/`
+
+## Test Organization
+
+DataFusion has several levels of tests in its [Test
+Pyramid](https://martinfowler.com/articles/practical-test-pyramid.html)
+and tries to follow [Testing Organization](https://doc.rust-lang.org/book/ch11-03-test-organization.html) in the The Book.
+
+This section highlights the most important test modules that exist
+
+### Unit tests
+
+Tests for the code in an individual module are defined in the same source file with a `test` module, following Rust convention
+
+
+### Rust Integration Tests
+
+There are several tests of the public interface of the DataFusion library in the [tests](https://github.com/apache/arrow-datafusion/blob/master/datafusion/tests) directory.
+
+You can run these tests individually using a command such as
+
+```shell
+cargo test -p datafusion --tests sql_integration
+```
+
+One very important test is the [sql_integraton](https://github.com/apache/arrow-datafusion/blob/master/datafusion/tests/sql_integration.rs) test which validates DataFusion's ability to run a large assortment of SQL queries against an assortment of data setsups.
+
+
+### SQL / Postgres Integration Tests
+
+The [integration-tests](https://github.com/apache/arrow-datafusion/blob/master/datafusion/integration-tests] directory contains a harness that runs certain queries against both postgres and datafusion and compares results
+
+
+#### setup environment
+```shell
Review comment:
@Jimexist is this ok instructions for invoking the integration test locally?
It would probably be nicer to make it a little easier (like put the table creation in sql and default some of the arguments) but perhaps we can do that as a follow on ticket
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb commented on pull request #1471: Add section on testing to the Developers doc
Posted by GitBox <gi...@apache.org>.
alamb commented on pull request #1471:
URL: https://github.com/apache/arrow-datafusion/pull/1471#issuecomment-1035613786
I was going through the PRs in datafusion and ran across this one. Given I am planning (hoping) we have more people interested in the project after the 7.0.0 release I wanted to get this documented a bit more
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] xudong963 commented on a change in pull request #1471: Add section on testing to the Developers doc
Posted by GitBox <gi...@apache.org>.
xudong963 commented on a change in pull request #1471:
URL: https://github.com/apache/arrow-datafusion/pull/1471#discussion_r773200343
##########
File path: DEVELOPERS.md
##########
@@ -40,6 +40,57 @@ Testing setup:
- `export PARQUET_TEST_DATA=$(pwd)/parquet-testing/data/`
- `export ARROW_TEST_DATA=$(pwd)/testing/data/`
+
+## Test Organization
+
+DataFusion has several levels of tests in its [Test
+Pyramid](https://martinfowler.com/articles/practical-test-pyramid.html)
+and tries to follow [Testing Organization](https://doc.rust-lang.org/book/ch11-03-test-organization.html) in the The Book.
+
+This section highlights the most important test modules that exist
+
+### Unit tests
+
+Tests for the code in an individual module are defined in the same source file with a `test` module, following Rust convention
+
+For example, the logic for pruning predicates is defined in [pruning.rs](https://github.com/apache/arrow-datafusion/blob/main/datafusion/src/physical_optimizer/pruning.rs#L708) using the
+
+```rust
+#[cfg(test)]
+mod tests {
+...
+}
+```
+
+
+### Rust Integration Tests
+
+There are several tests of the public interface of the DataFusion library in the [tests](https://github.com/apache/arrow-datafusion/blob/master/datafusion/tests) directory.
+
+You can run these tests individually using a command such as
+
+```shell
+cargo test -p datafusion --tests sql
+```
+
+The tests are:
Review comment:
Agree.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb merged pull request #1471: Add section on testing to the Developers doc
Posted by GitBox <gi...@apache.org>.
alamb merged pull request #1471:
URL: https://github.com/apache/arrow-datafusion/pull/1471
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] xudong963 commented on pull request #1471: Add section on testing to the Developers doc
Posted by GitBox <gi...@apache.org>.
xudong963 commented on pull request #1471:
URL: https://github.com/apache/arrow-datafusion/pull/1471#issuecomment-998840101
The descriptions of our existing tests are clear in the ticket, thanks @alamb.
I opened an issue about sqllogictest #1453 the other day. The reason why I opened it is because I think the `SQL / Postgres Integration Tests` isn't solid, it doesn't have a `test scale`. (Indeed Datafusion mainly relies on sql.rs for testing).
If I understand correctly, the test refactoring that @hntd187 and @matthewmturner are doing doesn't conflict with what I mentioned above. They mainly want to better classify existing tests, which should be unit tests and which should be integration tests. I think it makes sense to me, thanks @hntd187 @matthewmturner. Now we're used to putting tests directly in sql.rs, but sometimes it's better to put it in a source file with a test mod.
Back to my sqllogictest, I think if we can improve the scale of the `SQL / Postgres Integration Tests`, sqllogictest mentioned by me is not necessary.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] hntd187 commented on a change in pull request #1471: Add section on testing to the Developers doc
Posted by GitBox <gi...@apache.org>.
hntd187 commented on a change in pull request #1471:
URL: https://github.com/apache/arrow-datafusion/pull/1471#discussion_r773362444
##########
File path: DEVELOPERS.md
##########
@@ -40,6 +40,57 @@ Testing setup:
- `export PARQUET_TEST_DATA=$(pwd)/parquet-testing/data/`
- `export ARROW_TEST_DATA=$(pwd)/testing/data/`
+
+## Test Organization
+
+DataFusion has several levels of tests in its [Test
+Pyramid](https://martinfowler.com/articles/practical-test-pyramid.html)
+and tries to follow [Testing Organization](https://doc.rust-lang.org/book/ch11-03-test-organization.html) in the The Book.
+
+This section highlights the most important test modules that exist
+
+### Unit tests
+
+Tests for the code in an individual module are defined in the same source file with a `test` module, following Rust convention
+
+For example, the logic for pruning predicates is defined in [pruning.rs](https://github.com/apache/arrow-datafusion/blob/main/datafusion/src/physical_optimizer/pruning.rs#L708) using the
+
+```rust
+#[cfg(test)]
+mod tests {
+...
+}
+```
+
+
+### Rust Integration Tests
+
+There are several tests of the public interface of the DataFusion library in the [tests](https://github.com/apache/arrow-datafusion/blob/master/datafusion/tests) directory.
+
+You can run these tests individually using a command such as
+
+```shell
+cargo test -p datafusion --tests sql
+```
+
+The tests are:
Review comment:
I was going to grain it down a bit more with a module for sql but then specific modules separated by functionality such as "order_by", "group_by" things of that nature, where generally they fall under sql, but more specifically if they touch specific functionality they go deeper. I'm not sure what @matthewmturner has started so maybe I'll wait a day to see what he says.
I also wanted to separate tests for internal things but the unit tests inside individual code seem fine for that.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb commented on a change in pull request #1471: Add section on testing to the Developers doc
Posted by GitBox <gi...@apache.org>.
alamb commented on a change in pull request #1471:
URL: https://github.com/apache/arrow-datafusion/pull/1471#discussion_r774784755
##########
File path: DEVELOPERS.md
##########
@@ -40,6 +40,57 @@ Testing setup:
- `export PARQUET_TEST_DATA=$(pwd)/parquet-testing/data/`
- `export ARROW_TEST_DATA=$(pwd)/testing/data/`
+
+## Test Organization
+
+DataFusion has several levels of tests in its [Test
+Pyramid](https://martinfowler.com/articles/practical-test-pyramid.html)
+and tries to follow [Testing Organization](https://doc.rust-lang.org/book/ch11-03-test-organization.html) in the The Book.
+
+This section highlights the most important test modules that exist
+
+### Unit tests
+
+Tests for the code in an individual module are defined in the same source file with a `test` module, following Rust convention
+
+For example, the logic for pruning predicates is defined in [pruning.rs](https://github.com/apache/arrow-datafusion/blob/main/datafusion/src/physical_optimizer/pruning.rs#L708) using the
+
+```rust
+#[cfg(test)]
+mod tests {
+...
+}
+```
+
+
+### Rust Integration Tests
+
+There are several tests of the public interface of the DataFusion library in the [tests](https://github.com/apache/arrow-datafusion/blob/master/datafusion/tests) directory.
+
+You can run these tests individually using a command such as
+
+```shell
+cargo test -p datafusion --tests sql
+```
+
+The tests are:
Review comment:
> In addition a lot of tests are prepended with csv, which seems somewhat redundant to me, is there a purpose behind it?
Not that I can remember
> I think I will reorganize the tests themselves first, touch no code other than moving around and then we can talk about renaming or any actual code changes.
I think that is an excellent strategy 👍
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org