Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Refactor] Refactor unit test structure #433

Merged
merged 48 commits into from
Jan 27, 2021

Conversation

congee524
Copy link
Contributor

Refactor unittest structure according to design document

@codecov
Copy link

codecov bot commented Dec 11, 2020

Codecov Report

Merging #433 (c6a4d2b) into master (3a3e10a) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #433   +/-   ##
=======================================
  Coverage   84.76%   84.76%           
=======================================
  Files         121      121           
  Lines        8579     8579           
  Branches     1416     1416           
=======================================
  Hits         7272     7272           
  Misses        953      953           
  Partials      354      354           
Flag Coverage Δ
unittests 84.75% <100.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
mmaction/models/backbones/resnet3d.py 90.40% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3a3e10a...c6a4d2b. Read the comment docs.

@innerlee
Copy link
Contributor

What's the status now, is it ready for review?

@dreamerlin dreamerlin mentioned this pull request Dec 14, 2020
@congee524
Copy link
Contributor Author

yes. And we will then raise another pull request to extract the common function in mmcv

@dreamerlin
Copy link
Collaborator

Here are some suggestions to discuss:

  1. Data directory in tests/data should remove prefix test in the name of theirs, and we can create a tests/data/annotations directory to hold the annotation files.
  2. How about creating a base.py under tests/test_data/test_datasets, where we can define a class BaseTestDataset(cls) with def setup_class(cls) holding some basic info like cls.test_data_directory and cls.test_anno_directory to avoid dupilcated ../data for certain dataset by inheriting it. And this thought can be applied to other parts in the unittest.
  3. Same as 2. some frequently used function like generate_demo_inputs, _demo_inputs , etc. can be integrated and put in a base.py which can be inherit by other modules. (Note, these functions may be not able to put in MMCV due to the difference of the data among different repos)
  4. Some filenames should be plural, like test_common_module -> test_common_modules.
  5. Some file position is not correct. test_inference.py should be in test_runtime since it is more about the pipeline of the repo rather than a certain model. test_gradcam.py should be put in test_models since it is more about the model not the utils. test_onnx.py should be put in test_utils.py since it is more about a onnx conversion tools. etc.
  6. To reduce some duplicated code, you can refer to this, and try to use pytest.mark.parametrize for some codes.

What do you think ? @congee524 @innerlee

@congee524
Copy link
Contributor Author

Here are some suggestions to discuss:

  1. Data directory in tests/data should remove prefix test in the name of theirs, and we can create a tests/data/annotations directory to hold the annotation files.
  2. How about creating a base.py under tests/test_data/test_datasets, where we can define a class BaseTestDataset(cls) with def setup_class(cls) holding some basic info like cls.test_data_directory and cls.test_anno_directory to avoid dupilcated ../data for certain dataset by inheriting it. And this thought can be applied to other parts in the unittest.
  3. Same as 2. some frequently used function like generate_demo_inputs, _demo_inputs , etc. can be integrated and put in a base.py which can be inherit by other modules. (Note, these functions may be not able to put in MMCV due to the difference of the data among different repos)
  4. Some filenames should be plural, like test_common_module -> test_common_modules.
  5. Some file position is not correct. test_inference.py should be in test_runtime since it is more about the pipeline of the repo rather than a certain model. test_gradcam.py should be put in test_models since it is more about the model not the utils. test_onnx.py should be put in test_utils.py since it is more about a onnx conversion tools. etc.
  6. To reduce some duplicated code, you can refer to this, and try to use pytest.mark.parametrize for some codes.

What do you think ? @congee524 @innerlee

about the first point, does it means test_activity_features -> activity_features ?
Does the anotation files means hte anotation files of training, validation and testing?

@congee524 congee524 added the WIP work in progress label Dec 15, 2020
@dreamerlin
Copy link
Collaborator

test_activity_features -> activity_features

like test_activity_features -> activity_features

@congee524 congee524 removed the WIP work in progress label Dec 16, 2020
@dreamerlin dreamerlin mentioned this pull request Dec 17, 2020
15 tasks
@dreamerlin dreamerlin mentioned this pull request Jan 11, 2021
6 tasks
@innerlee
Copy link
Contributor

Is this ready for review & merge?

@congee524
Copy link
Contributor Author

Is this ready for review & merge?

I'm waiting for releasing of mmcv to apply its latest common testing function

@congee524 congee524 added WIP work in progress and removed BLOCKED BY MMCV labels Jan 22, 2021
@congee524 congee524 marked this pull request as draft January 22, 2021 02:43
@kennymckormick
Copy link
Member

I think est_tem_results should be removed, is it a typo?

@congee524
Copy link
Contributor Author

I think est_tem_results should be removed, is it a typo?

yeah, and I have removed it

@congee524
Copy link
Contributor Author

kindly ping :p @innerlee @SuX97

@@ -0,0 +1,10 @@
[
{
"video_dir": "rawvideo_dataset",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the difference between rawvideo and video? I think it should be consistent of dataset types.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RawVideo dataset is used in the Project OmniSource. They have some differences.

Copy link
Collaborator

@SuX97 SuX97 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. May need to check if those common check functions moved to mmcv.utils are actually being used.
  2. How to set a threshold to split the unittest?

.gitignore Outdated Show resolved Hide resolved
docs/changelog.md Outdated Show resolved Hide resolved
@congee524
Copy link
Contributor Author

  1. May need to check if those common check functions moved to mmcv.utils are actually being used.
  2. How to set a threshold to split the unittest?

I have checked it all before, but there may be some omissions. We may fix them in future PR If there exist some omissions.

There is no threshold for the time being. In fact, I have some confusion about test_localizers and test_backbones

@innerlee innerlee merged commit b747208 into open-mmlab:master Jan 27, 2021
@congee524 congee524 deleted the refactor_unittest_structure branch January 27, 2021 06:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants