Update tests #618
Update tests #618
Conversation
| from kaggle import api | ||
|
|
||
|
|
||
| # Unit test names include a letter to sort them in run order. |
There was a problem hiding this comment.
[no action required] - It looks like test order is important because some tests depend on previous tests? Ideally each test would be independent, but I don't think it's easy to disentangle this.
There was a problem hiding this comment.
Yes, for example, you need to create a dataset before you can update or download it. We could make tests that ran each function required, and that might be easier to maintain. But they would also run much more slowly. For now, I'll keep the dependencies.
tests/unit_tests.py
Outdated
| def tearDownModule(): | ||
| file = os.path.join(dataset_directory, api.DATASET_METADATA_FILE) | ||
| if os.path.exists(file): | ||
| os.remove(file) | ||
| if os.path.exists(api.DATASET_METADATA_FILE): | ||
| os.remove(api.DATASET_METADATA_FILE) |
There was a problem hiding this comment.
Why are these two files cleaned up globally vs as part of the test where they are created? It looks like other files are cleaned up per-test, so maybe this one should be too?
There was a problem hiding this comment.
They are required to run other tests. It turned out that I wasn't deleting some other metadata files, so I added them here.
tests/unit_tests.py
Outdated
| version_num = 0 | ||
| except FileNotFoundError: | ||
| version_num = 0 | ||
| pass |
There was a problem hiding this comment.
I think this can be removed.
tests/unit_tests.py
Outdated
| json.dump(meta_data, f, indent=2) | ||
| return meta_data | ||
|
|
||
| def create_dataset_metadata_file(dataset_dir): |
There was a problem hiding this comment.
nit - I don't think this really creates anything. IIUC, it just reads an existing file and returns the version number or 0 if the file doesn't exist? Does it need a different name?
There was a problem hiding this comment.
Changed to initialize_dataset_metadata_file().
tests/unit_tests.py
Outdated
| # KAGGLE_KEY=local_api_token | ||
| # KAGGLE_USERNAME=<kaggle-user-name> | ||
|
|
||
| model_owner = api.config_values['username'] |
There was a problem hiding this comment.
nit - I think this is used in the kernel tests as well, so maybe should be called something like test_user?
| self.test_dataset_b_metadata() | ||
| try: | ||
| api.dataset_metadata_update(self.dataset, None) | ||
| # TODO Make the API method return something, and not exit when it fails. |
There was a problem hiding this comment.
Is it possible to download the dataset metadata and make sure it was updated? At least it would be some sort of validation...
There was a problem hiding this comment.
The API doesn't have a function to download dataset metadata.
tests/unit_tests.py
Outdated
| print(new_dataset.error) # This is likely to happen, and that's OK. | ||
| except ApiException as e: | ||
| self.fail(f"dataset_create_new failed: {e}") | ||
| # finally: |
There was a problem hiding this comment.
Is there a reason why this cleanup is commented out?
tests/unit_tests.py
Outdated
| try: | ||
| ms = api.models_list() | ||
| self.assertIsInstance(ms['models'], list) | ||
| # self.assertGreater(ms['models'], 0) |
There was a problem hiding this comment.
Are there not any models? It seems like this check should be uncommented.
tests/unit_tests.py
Outdated
| try: | ||
| model = api.model_create_new('model') | ||
| if model.hasError: | ||
| # Handle potential errors gracefully (potentially skip the test) |
| except ApiException as e: | ||
| self.fail(f"model_instance_create failed: {e}") | ||
|
|
||
| def test_model_instance_b_wait_after_create(self): |
There was a problem hiding this comment.
I think this is OK temporarily, but it might be nice to leave a TODO for something better. I think the intention is to run this if b and c are running, but not if just b or c runs? One option could be to refactor the shared setup and include the time.sleep there?
There was a problem hiding this comment.
I added a TODO.
This exercises all the API functions, unlike the original, which only run a few. It might be best to use
View Fileto seepython_api_tests.py. (I didn't even look at the diff.)