search

py-mine / mcstatus

A Python library for checking the status of Minecraft servers
https://mcstatus.readthedocs.io
Apache License 2.0
478 stars 37 forks source link

tests: add basic end-to-end tests for CLI #865

Closed katrinafyi closed 2 months ago

katrinafyi commented 3 months ago

currently makes use of demo.mcstatus.io as a test server, but it would be good to run a local instance perhaps.

also adds output tests for --help, allowing this to be used in documentation if desired.

these tests are designed on top of #849 and will not make sense without it. this PR is separate for a more convenient reviewing experience.

i assure you the pre-commit checks pass when this commit is placed on top of #849.

katrinafyi commented 2 months ago

Perhaps.

From my perspective, test runtime is not too much of a problem. There are solutions such as running on CI only and batching tests I think something above unit tests is worthwhile and this is something at least. Tests and PRs need not be perfect, they just have to improve the situation.

What do you want to do with this? I admit my main motivation was to unblock the --help Readme problem, which obviously independent of the network. The PR could be stripped back to only that.

I will also not be offended if you reject this PR.

ItsDrike commented 2 months ago

I agree with @PerchunPak. Tests for the script should only test whether the script produces the output that we expect given some output from the lower api functions.

I'm not against adding integration tests, demo.mcstatus.io might actually be a good choice for this, but they shouldn't be a part of a "CLI tests" PR. If you wish, you can open another PR purely for integration tests, maybe adding them in tests/integration. These should then run against the public API functions like JavaServer.status, query, ... They should also be ran separately from the usual pytest suite and I'd like to see a dedicated github workflow action that runs alongside the multiplatform unit-tests for it.

As for this PR, let's just patch the responses from these functions. Unit-tests should test individual units, mostly independently of each other, we don't want the CLI unit tests to fail if the status method got broken, the CLI code would still be valid at that point, assuming status would do what it was expected to do.

katrinafyi commented 2 months ago

I guess this PR does mix end-to-end tests with some misc CLI tests. However, I do believe that testing the CLI represents the widest end-to-end span possible in this codebase, so I think it's appropriate to use for e2e tests (i.e. in place of the status methods). This PR was not intended to be unit tests.

I see you have big plans, which is very good. Unfortunately, I'm uninterested in and don't have the time to pursue such extensive work.

You can cherry-pick from this what you wish. The stdout/err capturing will be useful.

I wish you safe travels!

PerchunPak commented 2 months ago

I wish you safe travels!

Does this mean I can take this PR?

katrinafyi commented 2 months ago

Yes. It's probably easiest to copy the commits into a branch you control.

On Thu, 1 Aug 2024, 09:55 Perchun Pak, @.***> wrote:

I wish you safe travels!

Does this mean I can take this PR?

— Reply to this email directly, view it on GitHub https://github.com/py-mine/mcstatus/pull/865#issuecomment-2261676778, or unsubscribe https://github.com/notifications/unsubscribe-auth/AJNGQOQHBJ2LPCBYZKW5BA3ZPF2QFAVCNFSM6AAAAABLROK2VSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDENRRGY3TMNZXHA . You are receiving this because you authored the thread.Message ID: @.***>

PerchunPak commented 2 months ago

Closing in favor of #877