Documentation added for testing #1281

razajafri · 2020-12-04T22:38:05Z

This PR starts the process of improving our documentation for running integration and unit-tests

This addresses #814

Signed-off-by: Raza Jafri <rjafri@nvidia.com>

razajafri · 2020-12-04T22:38:44Z

@sameerz PTAL

CONTRIBUTING.md

Co-authored-by: Jason Lowe <jlowe@nvidia.com>

Addressed review comments

minor change to the example and fixed a broken link

razajafri · 2020-12-05T05:42:31Z

build

GaryShen2008 · 2020-12-05T06:55:43Z

build

razajafri · 2020-12-05T19:02:17Z

build

tgravescs · 2020-12-07T14:56:06Z

docs/dev/testing.md

+- To run test ParquetWriterSuite in package com.nvidia.spark.rapids, issue `mvn test -DwildcardSuites="com.nvidia.spark.rapids.ParquetWriterSuite"`
+
+
+#### Integration tests


so there is already documentation in integration_tests.md do we really need this in both places? or just have a link and put the details in that doc

I would prefer to have us link to it. There is also documentation about unit tests

https://github.com/NVIDIA/spark-rapids/blob/branch-0.3/docs/testing.md

Not sure why we are not linking to that primarily and updating it as needed.

I will link to the testing doc and update if needed. I honestly wasn't aware we have a document already, thanks for bringing it up

I'm still confused, why do we have so many different docs for testing. I realize you linked them but now there is info in testing.md and integrationtest.md . We added information here on running but I need to go to integration tests readme.md to know what i need to install to add it. why don't we put all information in the integration test readme and test readme and just have small description and point to those? Maybe i'm missing something @revans2 @jlowe opinions?

tgravescs · 2020-12-07T14:56:08Z

docs/dev/testing.md

+better insight into what is needed as we constantly keep working on to improve and expand the plugin-support.
+
+The tests are written python and run with pytest and the script honors pytest parameters. Some handy flags are:
+- `-k` <pytest-file-name>. This will run all the tests in that test file.


I guess I haven't tried this, usually I specify the path to the test file to run all tests in a file. I supposed both works? or is this specific to the run_pyspark_from_build.sh script? I have been doing spark-submit way.

-k does a test search for the name of the test. This includes the file name and the extensions for specific parameters. I prefer -k because I can do things like -k Decimal and I will typically end up running all of the tests that touch the decimal type either as a parameter or something else.

But it would be good to document both ways.

tgravescs · 2020-12-07T14:56:26Z

docs/dev/testing.md

+- `-k` <pytest-file-name>. This will run all the tests in that test file.
+- `-k` <test-name>. This will also run an individual test.
+- `-s` Doesn't capture the output and instead prints to the screen.
+- `-v` increase verbosity of the tests


perhaps add the debug tmp path option that save the output

are you referring to --debug? This option doesn't accept path. Are you referring to some other option?

revans2 · 2020-12-07T15:11:20Z

docs/dev/testing.md

@@ -0,0 +1,44 @@
+### Testing your code
+There are two types of tests in this project. Unit tests that are written in Scala, and integration tests that are written in Python. 


We actually have integration tests in both Scala and python.

revans2 · 2020-12-07T15:12:59Z

docs/dev/testing.md

+- To run test ParquetWriterSuite in package com.nvidia.spark.rapids, issue `mvn test -DwildcardSuites="com.nvidia.spark.rapids.ParquetWriterSuite"`
+
+
+#### Integration tests


I would prefer to have us link to it. There is also documentation about unit tests

https://github.com/NVIDIA/spark-rapids/blob/branch-0.3/docs/testing.md

Not sure why we are not linking to that primarily and updating it as needed.

revans2 · 2020-12-07T15:15:00Z

docs/dev/testing.md

+better insight into what is needed as we constantly keep working on to improve and expand the plugin-support.
+
+The tests are written python and run with pytest and the script honors pytest parameters. Some handy flags are:
+- `-k` <pytest-file-name>. This will run all the tests in that test file.


-k does a test search for the name of the test. This includes the file name and the extensions for specific parameters. I prefer -k because I can do things like -k Decimal and I will typically end up running all of the tests that touch the decimal type either as a parameter or something else.

But it would be good to document both ways.

razajafri · 2020-12-07T21:42:29Z

build

razajafri · 2020-12-07T21:47:25Z

build

razajafri · 2020-12-07T21:47:31Z

@tgravescs @revans2 I have addressed your concerns PTAL

revans2 · 2020-12-07T23:01:32Z

docs/testing.md

@@ -39,20 +39,54 @@ They generally follow TPCH but are not guaranteed to be the same.

 ## Unit tests

-Unit tests exist in the tests directory. This is unconventional and is done so we can run the tests
-on the final shaded version of the plugin. It also helps with how we collect code coverage.
+Unit tests exist in the [here](tests) directory. This is unconventional and is done so we can run


"unit tests exist in the here directory" is not very readable. Also not sure why you are linking to. There is no tests directory or file under docs that this can or should link to.

revans2 · 2020-12-07T23:03:33Z

docs/testing.md

+Unit tests exist in the [here](tests) directory. This is unconventional and is done so we can run
+ the tests on the final shaded version of the plugin. It also helps with how we collect code coverage.
+
+in order to run the unit-tests follow these steps


Please capitalize sentences.

revans2 · 2020-12-07T23:05:45Z

docs/testing.md

+
+in order to run the unit-tests follow these steps
+1. issue the maven command to run the tests with `mvn test`. this will run all the tests
+2. to run individual tests append `-dwildcardsuites=<comma separated list of wildcard suite names to execute>` to the above command 


These are not steps. They are options and I don't think we need a list to express them.

revans2 · 2020-12-07T23:06:54Z

docs/testing.md

+1. issue the maven command to run the tests with `mvn test`. this will run all the tests
+2. to run individual tests append `-dwildcardsuites=<comma separated list of wildcard suite names to execute>` to the above command 
+
+for more information about using scalatest with maven please refere [here](https://www.scalatest.org/user_guide/using_the_scalatest_maven_plugin)


I'm not known for my spelling but I don't think refer has an "e" at the end

revans2 · 2020-12-07T23:08:20Z

docs/testing.md

-There are two frameworks used for testing. One is based off of pytest and pyspark in the 
-`src/main/python` directory. These tests will run as a part of the maven build if you have the environment
-variable `SPARK_HOME` set.
+Integration tests are stored in the [integration_tests](integration_tests) directory. 


This is not in the right directory for this link to work. Please change it back to the README or at least ../integration_tests

revans2 · 2020-12-07T23:08:47Z

docs/testing.md

@@ -117,3 +151,12 @@ Next you can start to run the tests.
 durations.run(new com.nvidia.spark.rapids.JoinsSuite)
 ...
 ```
+
+For more details on Integration tests options please refer to integration-test [doc](integration-tests/README.md)


Need to test out the links I don't think this works.

razajafri · 2020-12-07T23:38:16Z

@revans2 better now? The links were all copied from where I added this section originally in the CONTRIBUTING.md.

Thanks for reviewing

razajafri · 2020-12-07T23:38:21Z

build

tgravescs · 2020-12-08T15:15:14Z

docs/testing.md

+
+In order to run the unit-tests issue the maven command to run the tests i.e. `mvn test`. 
+
+To run individual tests append `-DwildcardSuites=<comma separated list of wildcard suite names to execute>` to the above command 


does this work for both scala and java tests? I know the java ones are different in spark

wildcardSuites only applies to scalatest. Java tests need to be filtered via surefire's approach which is -Dtest=...

right so we have a java test right - TestHashedPriorityQueue.java? so adding how to run it would be useful and specifying here this works for scala tests.

I guess this text is old new one mentions scala tests

tgravescs · 2020-12-08T15:18:44Z

docs/dev/testing.md

+- To run test ParquetWriterSuite in package com.nvidia.spark.rapids, issue `mvn test -DwildcardSuites="com.nvidia.spark.rapids.ParquetWriterSuite"`
+
+
+#### Integration tests


I'm still confused, why do we have so many different docs for testing. I realize you linked them but now there is info in testing.md and integrationtest.md . We added information here on running but I need to go to integration tests readme.md to know what i need to install to add it. why don't we put all information in the integration test readme and test readme and just have small description and point to those? Maybe i'm missing something @revans2 @jlowe opinions?

docs/testing.md

jlowe · 2020-12-08T15:28:46Z

docs/testing.md

+Unit tests exist in the [tests](../tests) directory. This is unconventional and is done so we can
+ run the tests on the final shaded version of the plugin. It also helps with how we collect code coverage.
+
+In order to run the unit-tests issue the maven command to run the tests i.e. `mvn test`. 


Suggested change

In order to run the unit-tests issue the maven command to run the tests i.e. `mvn test`.

Use Maven to run the unit tests via `mvn test`.

jlowe · 2020-12-08T15:29:35Z

docs/testing.md

+
+In order to run the unit-tests issue the maven command to run the tests i.e. `mvn test`. 
+
+To run individual tests append `-DwildcardSuites=<comma separated list of wildcard suite names to execute>` to the above command 


wildcardSuites only applies to scalatest. Java tests need to be filtered via surefire's approach which is -Dtest=...

docs/testing.md

jlowe · 2020-12-08T15:34:28Z

docs/testing.md

+apache spark specific configurations can be passed in by setting environment-variable spark_conf 
+
+Examples: 
+-To run tests against Apache Spark 3.1.0, 


This says it will run tests against Spark 3.1.0 but only runs one test. Either fix the description or fix the example.

docs/testing.md

jlowe · 2020-12-08T15:39:20Z

why don't we put all information in the integration test readme and test readme and just have small description and point to those?

I'm all for not repeating information across the various docs and instead linking. I agree we should keep unit test info to the unit test docs and integration test info to the integration test docs. Each doc can reference the other for more information on running the unit tests vs. integration tests.

revans2 · 2020-12-08T15:44:45Z

I'm all for not repeating information across the various docs and instead linking. I agree we should keep unit test info to the unit test docs and integration test info to the integration test docs. Each doc can reference the other for more information on running the unit tests vs. integration tests.

I'm not a big fan of pointing things from the docs directory outside of it, simply because the docs directory gets moved around when it becomes the github pages site. But because this is docs for developers I think @jlowe is right and we should have it all in the README for the various directories with links to them, so long as we are careful to make sure that the links work on the github pages site and in markdown.

razajafri · 2020-12-08T22:16:09Z

@revans2 @tgravescs @jlowe

Sorry about the back and forth. I have incorporated all the suggestions I think. Please check if this looks good. I have also added a section on the project README.md for tests and linked it to tests/README.md. Let me know if that looks good

razajafri · 2020-12-08T22:16:15Z

build

razajafri · 2020-12-09T00:24:42Z

build

razajafri · 2020-12-09T17:30:05Z

@revans2 @tgravescs do you have any other suggestions or does this look OK to go in?

CONTRIBUTING.md

tests/README.md

Co-authored-by: Jason Lowe <jlowe@nvidia.com>

razajafri · 2020-12-09T17:50:36Z

build

razajafri · 2020-12-09T18:09:40Z

@jlowe thanks for the suggestions. They have all been applied. Please take another look

jlowe

This looks OK to me, but @revans2 and @tgravescs should comment before merging.

tgravescs

overall is fine, I think we have one java unit test that we could say how to run individually but not a big deal

razajafri · 2020-12-09T19:16:42Z

Thanks, @tgravescs I have created a follow-on #1345

* documentation added for testing Signed-off-by: Raza Jafri <rjafri@nvidia.com> * Capitalize word Co-authored-by: Jason Lowe <jlowe@nvidia.com> * Update CONTRIBUTING.md Addressed review comments * Update CONTRIBUTING.md minor change to the example and fixed a broken link * created a separate document for testing * addressed review comments * minor fix * fixed broken links and capitalization * addressed review comments * fix broken link * exclude README.md from rat * Update CONTRIBUTING.md Co-authored-by: Jason Lowe <jlowe@nvidia.com> * Update CONTRIBUTING.md Co-authored-by: Jason Lowe <jlowe@nvidia.com> * Update tests/README.md Co-authored-by: Jason Lowe <jlowe@nvidia.com> * Update tests/README.md Co-authored-by: Jason Lowe <jlowe@nvidia.com> * Update tests/README.md Co-authored-by: Jason Lowe <jlowe@nvidia.com> * Update tests/README.md Co-authored-by: Jason Lowe <jlowe@nvidia.com> Co-authored-by: Raza Jafri <rjafri@nvidia.com> Co-authored-by: Jason Lowe <jlowe@nvidia.com>

Signed-off-by: Tim Liu <timl@nvidia.com>

documentation added for testing

a90286e

Signed-off-by: Raza Jafri <rjafri@nvidia.com>

razajafri force-pushed the doc_updated branch from 0b01d85 to a90286e Compare December 4, 2020 22:38

sameerz added the documentation Improvements or additions to documentation label Dec 4, 2020

jlowe requested changes Dec 5, 2020

View reviewed changes

razajafri and others added 4 commits December 4, 2020 17:54

Capitalize word

ca84f16

Co-authored-by: Jason Lowe <jlowe@nvidia.com>

Update CONTRIBUTING.md

e2e90db

Addressed review comments

Update CONTRIBUTING.md

2f72472

minor change to the example and fixed a broken link

created a separate document for testing

53c80f2

Merge remote-tracking branch 'origin/branch-0.3' into test_doc_1

21dcdd7

tgravescs reviewed Dec 7, 2020

View reviewed changes

revans2 reviewed Dec 7, 2020

View reviewed changes

addressed review comments

bcedd29

minor fix

d07e6fb

revans2 reviewed Dec 7, 2020

View reviewed changes

fixed broken links and capitalization

a606277

tgravescs reviewed Dec 8, 2020

View reviewed changes

jlowe requested changes Dec 8, 2020

View reviewed changes

razajafri added 2 commits December 8, 2020 14:03

addressed review comments

dd5b71c

fix broken link

5a1ecfa

exclude README.md from rat

735b730

razajafri requested review from GaryShen2008 and NvTimLiu as code owners December 9, 2020 00:24

jlowe requested changes Dec 9, 2020

View reviewed changes

razajafri and others added 6 commits December 9, 2020 09:48

Update CONTRIBUTING.md

1839cc5

Co-authored-by: Jason Lowe <jlowe@nvidia.com>

Update CONTRIBUTING.md

6a9f8ea

Co-authored-by: Jason Lowe <jlowe@nvidia.com>

Update tests/README.md

066b051

Co-authored-by: Jason Lowe <jlowe@nvidia.com>

Update tests/README.md

881b635

Co-authored-by: Jason Lowe <jlowe@nvidia.com>

Update tests/README.md

e69567c

Co-authored-by: Jason Lowe <jlowe@nvidia.com>

Update tests/README.md

37f1721

Co-authored-by: Jason Lowe <jlowe@nvidia.com>

jlowe approved these changes Dec 9, 2020

View reviewed changes

revans2 approved these changes Dec 9, 2020

View reviewed changes

tgravescs approved these changes Dec 9, 2020

View reviewed changes

razajafri merged commit 714b063 into NVIDIA:branch-0.3 Dec 9, 2020

razajafri deleted the doc_updated branch December 9, 2020 19:16

sameerz added this to the Dec 7 - Dec 18 milestone Dec 26, 2020

tgravescs pushed a commit to tgravescs/spark-rapids that referenced this pull request Nov 30, 2023

Auto merge to branch-23.10 from branch-23.08 (NVIDIA#1281)

236e3c7

Signed-off-by: Tim Liu <timl@nvidia.com>

		- To run test ParquetWriterSuite in package com.nvidia.spark.rapids, issue `mvn test -DwildcardSuites="com.nvidia.spark.rapids.ParquetWriterSuite"`


		#### Integration tests

		@@ -0,0 +1,44 @@
		### Testing your code
		There are two types of tests in this project. Unit tests that are written in Scala, and integration tests that are written in Python.


		In order to run the unit-tests issue the maven command to run the tests i.e. `mvn test`.

		To run individual tests append `-DwildcardSuites=<comma separated list of wildcard suite names to execute>` to the above command

	In order to run the unit-tests issue the maven command to run the tests i.e. `mvn test`.
	Use Maven to run the unit tests via `mvn test`.

Documentation added for testing #1281

Documentation added for testing #1281

Conversation

razajafri commented Dec 4, 2020

razajafri commented Dec 4, 2020

razajafri commented Dec 5, 2020

GaryShen2008 commented Dec 5, 2020

razajafri commented Dec 5, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

razajafri commented Dec 7, 2020

razajafri commented Dec 7, 2020

razajafri commented Dec 7, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

razajafri commented Dec 7, 2020

razajafri commented Dec 7, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jlowe commented Dec 8, 2020

revans2 commented Dec 8, 2020

razajafri commented Dec 8, 2020

razajafri commented Dec 8, 2020

razajafri commented Dec 9, 2020

razajafri commented Dec 9, 2020

razajafri commented Dec 9, 2020

razajafri commented Dec 9, 2020

jlowe left a comment

Choose a reason for hiding this comment

tgravescs left a comment

Choose a reason for hiding this comment

razajafri commented Dec 9, 2020