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

Addressing issues identified in issue #5944 #5945

Closed
wants to merge 11 commits into from

Conversation

DeanDro
Copy link

@DeanDro DeanDro commented Jun 4, 2023

Hi,

I noticed a few issues in the statscollectors.py file and in the test suites in test_stats.py and test_link.py. In particular, the statscollector.py file had two methods that haven't been implemented in the StatsCollector class. These were the open_spinder() and the _persist_spider() methods. I did my best in completing these methods based on the documentation.
Additionally in the same file I noticed that the class DummyStatsCollector has no real functionality and doesn't have any implementation in the project outside the test suite. Thus, I decided that the best way to approach this is to remove the class from the file to reduce the size of the file.

For the two test suites in test_stats.py and test_link.py, I noticed a few issues:
In the test_stats.py:

  • Not all methods in StatsCollector class are tested. I have filled in for the missing methods in order to achieve 100% coverage.
  • There are two test cases that have multiple assert methods. This is hard to maintain, so I have separated the assert evaluations to a unique test case per evaluation and added comments.
  • I removed any tests from the DummyStatsCollector class, since this class had no real use and was removed from the statscollectors.py file.

In the test_link.py, this file didn't have any missing tests and all methods in the link.py file were evaluated; however, the way it was structured it was hard to maintain. Similar to the previous testing file, I have separate each method evaluation to a unique test case and added comments to each test case.

All test cases are evaluated as True when I test the updated code.

Last but not least, there are several empty files in the project that I removed to reduce clutter. These were located at:

  • tests/test_dmcline_crawl_with_pipeline/test_spider/spiders/init.py
  • tests/test_utils_misc/test_walk_modules/init.py
  • tests/test_utils_misc/test_walk_modules/mod1.py

Thanks!

@wRAR
Copy link
Member

wRAR commented Jun 4, 2023

the statscollector.py file had two methods that haven't been implemented in the StatsCollector class

This is intended.

the class DummyStatsCollector has no real functionality and doesn't have any implementation in the project outside the test suite

This is intended. It's even documented.

I decided that the best way to approach this is to remove the class

This is wrong.

Not all methods in StatsCollector class are tested. I have filled in for the missing methods in order to achieve 100% coverage.

Please describe which methods were not tested and are tested now.

I removed any tests from the DummyStatsCollector class, since this class had no real use and was removed from the statscollectors.py file.

This is wrong.

Last but not least, there are several empty files in the project that I removed to reduce clutter.

Please don't remove random empty files, especially __init__.py files.

Additionally, please don't submit several unrelated changes as one PR and please don't submit random (and wrong) whitespace changes. Additionally, you have changed tests/test_utils_display.py without mentioning it.

@wRAR
Copy link
Member

wRAR commented Jun 4, 2023

Also, while splitting tests with many asserts in one method may make sense, writing long docstrings (especially wrong ones) for trivial self-documented comparisons IMO does not.

@DeanDro
Copy link
Author

DeanDro commented Jun 4, 2023

Hey,
The methods from the statscollectors.py that weren’t tested before but they are now are the set_stats and the clear_stats.

@DeanDro
Copy link
Author

DeanDro commented Jun 4, 2023

Apologizes for not mentioning the changes in the tests/test_utils_display.py before.
I ended up reverting back to the original version of the file before submitting, so I thought the changes weren’t captured.

@wRAR wRAR closed this Jun 21, 2023
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

2 participants