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.

Sign up for GitHub

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

Jump to bottom

Fix multiple issues in tests #358

Merged
merged 2 commits into from
Aug 15, 2022
Merged

Fix multiple issues in tests #358

merged 2 commits into from
Aug 15, 2022

Conversation

PerchunPak
Copy link
Member

@PerchunPak PerchunPak commented Jul 17, 2022 •
edited
Loading

This includes:

  • Move all tests from mcstatus folder, so they now won't be in coverage reports and in releases.
  • Rename for some classes, so they will be collected by pytest.
  • Change path for formatters, linters etc. to . instead of ./mcstatus. (Tests also should be formatted and checked)

And the second commit reduces the minimum coverage to 70%, I created #371 to bring it back to 80%.

ItsDrike
ItsDrike requested changes Jul 17, 2022
View reviewed changes
Copy link
Member

@ItsDrike ItsDrike left a comment

Choose a reason for hiding this comment

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

As far as I'm aware, we do not track coverage of the tests/ folder itself, pytest settings in pyproject.toml specify --cov=mcstatus, meaning we only take coverage from the mcstatus/ folder into consideration in the reports. What's the purpose of adding no cover comments into the tests themselves?

mcstatus/tests/test_retry_decorator.py Outdated Show resolved Hide resolved
mcstatus/tests/test_deprecated_decorator.py Outdated Show resolved Hide resolved
@ItsDrike ItsDrike added the area: tests Related to unit-tests or other type of testing label Jul 17, 2022
@PerchunPak
Copy link
Member Author

As far as I'm aware, we do not track coverage of the tests/ folder itself, pytest settings in pyproject.toml specify --cov=mcstatus, meaning we only take coverage from the mcstatus/ folder into consideration in the reports. What's the purpose of adding no cover comments into the tests themselves?

All tests in mcstatus/tests, so they go to coverage report. Reason is I don't know, just saw some dead code, and wanted to fix it.

@PerchunPak PerchunPak force-pushed the increase-coverage-in-tests-folder branch 2 times, most recently from 20cad3b to b9cabb9 Compare July 17, 2022 21:48
ItsDrike
ItsDrike requested changes Jul 17, 2022
View reviewed changes
Copy link
Member

@ItsDrike ItsDrike left a comment

Choose a reason for hiding this comment

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

All tests in mcstatus/tests, so they go to coverage report. Reason is I don't know, just saw some dead code, and wanted to fix it.

Wouldn't it then be a better idea to just change the pytest settings to not report coverage in tests?

@PerchunPak
Copy link
Member Author

Wouldn't it then be a better idea to just change the pytest settings to not report coverage in tests?

Yes. But this isn't a goal of this PR, this PR just fixes some dead tests.

@PerchunPak PerchunPak force-pushed the increase-coverage-in-tests-folder branch from b9cabb9 to f1786f7 Compare July 17, 2022 21:57
@ItsDrike
Copy link
Member

Yes. But this isn't a goal of this PR, this PR just fixes some dead tests.

According to PR's title, Increase coverage in tests/ folder the goal seems to be to improve the coverage purely in tests/, which I'd consider as a fundamentally flawed goal since I don't think we should even be reporting coverage in tests/ folder.

@PerchunPak
Copy link
Member Author

According to PR's title, Increase coverage in tests/ folder the goal seems to be to improve the coverage purely in tests/, which I'd consider as a fundamentally flawed goal since I don't think we should even be reporting coverage in tests/ folder.

I can't describe in another words why I'm documenting some test's parts with # pragma: ..., so... I of course could remove coverage, but this PR introducing also fix for not runned tests, which was ignored by pytest collector, so let's firstly merge it.

@ItsDrike
Copy link
Member

but this PR introducing also fix for not runned tests, which was ignored by pytest collector, so let's firstly merge it.

I agree, but let's not include the pragma ignores in here and only fix the issues with tests naming, you can rename the PR to Fix names of some test classes or something similar. Or, if you wish to do some more things here too, you can simply use a name like Fix issues in unit-tests, and you could also handle renaming, excluding tests/ from coverage, and any other issues in tests in a single place then.

@PerchunPak PerchunPak force-pushed the increase-coverage-in-tests-folder branch from f1786f7 to 62efe7d Compare July 18, 2022 17:50
@PerchunPak PerchunPak changed the title Increase coverage in tests/ folder Fix multiple issues in tests Jul 18, 2022
ItsDrike
ItsDrike requested changes Jul 18, 2022
View reviewed changes
tests/test_timeout.py Outdated Show resolved Hide resolved
@PerchunPak PerchunPak force-pushed the increase-coverage-in-tests-folder branch 2 times, most recently from a2dcf43 to 3f16c13 Compare July 18, 2022 18:36
@PerchunPak
Copy link
Member Author

tests/protocol/__init__.py was added by the IDE. I can remove it, if required.

@ItsDrike
Copy link
Member

tests/protocol/__init__.py was added by the IDE. I can remove it, if required.

It's ok, in fact it probably should be there.

kevinkjt2000
kevinkjt2000 requested changes Aug 6, 2022
View reviewed changes
Copy link
Contributor

@kevinkjt2000 kevinkjt2000 left a comment

Choose a reason for hiding this comment

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

Overall looks good. Only some small change requests from me.

tests/protocol/test_connection.py Outdated Show resolved Hide resolved
tests/test_timeout.py Outdated Show resolved Hide resolved
@PerchunPak PerchunPak force-pushed the increase-coverage-in-tests-folder branch 2 times, most recently from 660f601 to edd0c7d Compare August 11, 2022 10:10
@PerchunPak
Fix multiple issues in tests
10e9592 
This includes:
- Move all tests from `mcstatus` folder, so they now won't be in coverage
  reports and in releases.
- Rename for some classes, so they will be collected by pytest.
- Change path for formatters, linters etc. to `.` instead of `./mcstatus`.
  (Tests also should be formatted and checked)
@PerchunPak PerchunPak force-pushed the increase-coverage-in-tests-folder branch from edd0c7d to 10e9592 Compare August 11, 2022 10:13
@PerchunPak
Copy link
Member Author

Because of removing tests from coverage report, coverage is now 79% (and it's lower than 80%, so tests are failing).

@ItsDrike
Copy link
Member

Because of removing tests from coverage report, coverage is now 79% (and it's lower than 80%, so tests are failing).

I think we can drop the requirement to 70% for now

@PerchunPak PerchunPak mentioned this pull request Aug 13, 2022
Open
@ItsDrike ItsDrike merged commit be72306 into py-mine:master Aug 15, 2022
@PerchunPak PerchunPak deleted the increase-coverage-in-tests-folder branch August 15, 2022 17:12
PerchunPak added a commit to PerchunPak/mcstatus that referenced this pull request Aug 24, 2022
@PerchunPak
Move mcstatus/tests/status_response to /tests
163f2fd 
As it did py-mine#358
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ItsDrike ItsDrike ItsDrike approved these changes

@kevinkjt2000 kevinkjt2000 kevinkjt2000 approved these changes

Assignees
No one assigned
Labels
area: tests Related to unit-tests or other type of testing
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@PerchunPak @ItsDrike @kevinkjt2000