[Tests] General Test Design for all dataset scripts
State: closed | Author: patrickvonplaten (CONTRIBUTOR)

The general idea is similar to how testing is done in `transformers`. There is one general `test_dataset_common.py` file which has a `DatasetTesterMixin` class. This class implements all of the logic that can be used in a generic way for all dataset classes. The idea is to keep each individual dataset test file as minimal as possible. 

In order to test whether the specific data set class can download the data and generate the examples **without** downloading the actual data all the time, a MockDataLoaderManager class is used which receives a `mock_folder_structure_fn` function from each individual dataset test file that create "fake" data and which returns the same folder structure that would have been created when using the real data downloader. 

Comments: > I think I'm fine with this.
> 
> The alternative would be to host a small subset of the dataset on the S3 together with the testing script. But I think having all (test file creation + actual tests) in one file is actually quite convenient.
> 
> Good for me!
> 
> One question though, will we have to create one test file for each of the 100+ datasets or could we make some automatic conversion from tfds dataset test files?

I think if we go the way shown in the PR we would have to create one test file for each of the 100+ datasets. 

As far as I know the tfds test files all rely on the user having created a special download folder structure in `tensorflow-datasets/tensorflow_datasets/testing/test_data/fake_examples`. 

My hypothesis was: 
Becasue, we don't want to work with PRs, no `dataset_script` is going to be in the official repo, so no `dataset_script_test` can be in the repo either. Therefore we can also not have any "fake" test folder structure in the repo. 

**BUT:** As you mentioned @thom, we could have a fake data structure on AWS. To add a test the user has to upload multiple small test files when uploading his data set script. 

So for a cli this could look like:
`python nlp-cli upload <data_set_script> --testfiles <relative path to test file 1> <relative path to test file 2> ...` 

or even easier if the user just creates the dataset folder with the script inside and the testing folder structure, then the API could look like:

`python nlp-cli upload <path/to/dataset/folder>`

and the dataset folder would look like
```
squad
- squad.py
- fake_data  # this dir would have to have the exact same structure we get when downloading from the official squad data url
```

This way I think we wouldn't even need any test files at all for each dataset script. For special datasets like `c4` or `wikipedia` we could then allow to optionally upload another test script. 
We just assume that this is our downloaded `url` and check all functionality from there. 

Thinking a bit more about this solution sounds a) much less work and b) even easier for the user.

A small problem I see here though:
1) What do we do when the depending on the config name the downloaded folder structure is very different? I think for each dataset config name we should have one test, which could correspond to one "fake" folder structure on AWS

@thomwolf What do you think? I would actually go for this solution instead now.
@mariamabarham You have written many more tfds dataset scripts and tests than I have - what do you think? 



Regarding the tfds tests, I don't really see a point in keeping them because:

1) If you provide a fake data structure, IMO there is no need for each dataset to have an individual test file because (I think) most datasets have the same functions `_split_generators` and `_generate_examples` for which you can just test the functionality in a common test file. For special functions like these beam / pipeline functionality you probably need an extra test file. But @mariamabarham I think you have seen more than I have here as well 

2) The dataset test design is very much intertwined with the download manager design and contains a lot of code. I would like to seperate the tests into a) tests for downloading in general b) tests for post download data set pre-processing. Since we are going to change the download code anyways quite a lot, my plan was to focus on b) first.  

I like the idea of having a fake data folder on S3. I have seen datasets with nested compressed files structures that would be tedious to generate with code. And for users it is probably easier to create a fake data folder by taking a subset of the actual data, and then upload it as you said.

> > I think I'm fine with this.
> > The alternative would be to host a small subset of the dataset on the S3 together with the testing script. But I think having all (test file creation + actual tests) in one file is actually quite convenient.
> > Good for me!
> > One question though, will we have to create one test file for each of the 100+ datasets or could we make some automatic conversion from tfds dataset test files?
> 
> I think if we go the way shown in the PR we would have to create one test file for each of the 100+ datasets.
> 
> As far as I know the tfds test files all rely on the user having created a special download folder structure in `tensorflow-datasets/tensorflow_datasets/testing/test_data/fake_examples`.
> 
> My hypothesis was:
> Becasue, we don't want to work with PRs, no `dataset_script` is going to be in the official repo, so no `dataset_script_test` can be in the repo either. Therefore we can also not have any "fake" test folder structure in the repo.
> 
> **BUT:** As you mentioned @thom, we could have a fake data structure on AWS. To add a test the user has to upload multiple small test files when uploading his data set script.
> 
> So for a cli this could look like:
> `python nlp-cli upload <data_set_script> --testfiles <relative path to test file 1> <relative path to test file 2> ...`
> 
> or even easier if the user just creates the dataset folder with the script inside and the testing folder structure, then the API could look like:
> 
> `python nlp-cli upload <path/to/dataset/folder>`
> 
> and the dataset folder would look like
> 
> ```
> squad
> - squad.py
> - fake_data  # this dir would have to have the exact same structure we get when downloading from the official squad data url
> ```
> 
> This way I think we wouldn't even need any test files at all for each dataset script. For special datasets like `c4` or `wikipedia` we could then allow to optionally upload another test script.
> We just assume that this is our downloaded `url` and check all functionality from there.
> 
> Thinking a bit more about this solution sounds a) much less work and b) even easier for the user.
> 
> A small problem I see here though:
> 
> 1. What do we do when the depending on the config name the downloaded folder structure is very different? I think for each dataset config name we should have one test, which could correspond to one "fake" folder structure on AWS
> 
> @thomwolf What do you think? I would actually go for this solution instead now.
> @mariamabarham You have written many more tfds dataset scripts and tests than I have - what do you think?

I'm agreed with you just one thing, for some dataset like glue or xtreme you may have multiple datasets in it. so  I think a good way is to have one main fake folder and a subdirectory for each dataset inside

> Regarding the tfds tests, I don't really see a point in keeping them because:
> 
> 1. If you provide a fake data structure, IMO there is no need for each dataset to have an individual test file because (I think) most datasets have the same functions `_split_generators` and `_generate_examples` for which you can just test the functionality in a common test file. For special functions like these beam / pipeline functionality you probably need an extra test file. But @mariamabarham I think you have seen more than I have here as well
> 2. The dataset test design is very much intertwined with the download manager design and contains a lot of code. I would like to seperate the tests into a) tests for downloading in general b) tests for post download data set pre-processing. Since we are going to change the download code anyways quite a lot, my plan was to focus on b) first.

For _split_generator, yes. But I'm not sure for _generate_examples because there is lots of things that should be taken into account such as feature names and  types, data format (json, jsonl,  csv, tsv,..)

Sounds good to me!

When testing, we could thus just override the prefix in the URL inside the download manager to have them point to the test directory on our S3.

Cc @lhoestq 

Ok, here is a second draft for the testing structure. 

I think the big difficulty here is "How can you generate tests on the fly from a given dataset name, *e.g.* `squad`"?

So, this morning I did some research on "parameterized testing" and pure `unittest` or `pytest` didn't work very well. 
I found the lib https://github.com/wolever/parameterized, which works very nicely for our use case I think. 
@thomwolf - would it be ok to have a dependence on this lib for `nlp`? It seems like a light-weight lib to me. 

This lib allows to add a `parameterization` decorator to a `unittest.TestCase` class so that the class can be instantiated for multiple different arguments (which are the dataset names `squad` etc. in our case).

What I like about this lib is that one only has to add the decorator and the each of the parameterized tests are shown, like this: 

![Screenshot from 2020-04-24 15-13-14](https://user-images.githubusercontent.com/23423619/80216326-2bd9a680-863e-11ea-8a0f-460976f5309c.png)

With this structure we would only have to upload the dummy data for each dataset and would not require a specific testing file. 

What do you think @thomwolf @mariamabarham @lhoestq ?

I think this is a nice solution.

Do you think we could have the `parametrized` dependency in a `[test]` optional installation of `setup.py`? I would really like to keep the dependencies of the standard installation as small as possible. 

> I think this is a nice solution.
> 
> Do you think we could have the `parametrized` dependency in a `[test]` optional installation of `setup.py`? I would really like to keep the dependencies of the standard installation as small as possible.

Yes definitely!

UPDATE: 

This test design is ready now. I added dummy data to S3 for the dataests: `squad, crime_and_punish, sentiment140` . The structure can be seen on `https://s3.console.aws.amazon.com/s3/buckets/datasets.huggingface.co/nlp/squad/dummy/?region=us-east-1&tab=overview` for `squad`. 

All dummy data files have to be in .zip format and called `dummy_data.zip`. The zip file should thereby have the exact same folder structure one gets from downloading the "real" data url(s). 

To show how the .zip file looks like for the added datasets, I added the folder `nlp/datasets/dummy_data` in this PR. I think we can leave for the moment so that people can see better how to add dummy data tests and later delete it like `nlp/datasets/nlp`.