search

marcusbuffett / command-line-chess

A python program to play chess against an AI in the terminal.
MIT License
496 stars 156 forks source link

Added test, pytest configuration, removed unnecessary import #46

Closed GrbavaCigla closed 1 year ago

GrbavaCigla commented 1 year ago

Resolves #34

GrbavaCigla commented 1 year ago

Please let me know if you have an idea for more tests.

ClasherKasten commented 1 year ago

Please let me know if you have an idea for more tests.

Ideally I want to have a test coverage of 100%. So simply the idea would be to write tests for every part of the code that isn't covered by the tests. This doesn't have to be achieved with one PR, but this is the goal. Please tell me if you want to push further commits that add new tests to this PR or if I should merge it after the changes and you open more PRs for new tests (but please don't open a new PR for every single test).

GrbavaCigla commented 1 year ago

Thanks for the review.

Also why main.py instead of __main__.py?

ClasherKasten commented 1 year ago

Also why main.py instead of __main__.py?

main.py is just a normal python module and can be used like one. __main__.py has a special meaning for python. I think simplified __main__.py turns a directory into an executable python module. As an example suppose you have a directory called some_dir with an __main__.py file which contains all the startup code for the application. You could then use python -m some_dir to run you application.

GrbavaCigla commented 1 year ago

Also why main.py instead of __main__.py?

main.py is just a normal python module and can be used like one. __main__.py has a special meaning for python. I think simplified __main__.py turns a directory into an executable python module. As an example suppose you have a directory called some_dir with an __main__.py file which contains all the startup code for the application. You could then use python -m some_dir to run you application.

Exactly, shouldn't this module be executable since project is cli chess?

Anyways, I will fix this tomorrow since it is getting late.

ClasherKasten commented 1 year ago

Exactly, shouldn't this module be executable since project is cli

If you would've taken a look into README.md in the sections Installation and Usage it is written down how to properly execute it. But yeah, a __main__.py is an option to. So if you want to add, feel free to do so.

GrbavaCigla commented 1 year ago

Can you tell me what functions should I unittest because in testMate I am testing isCheckmate?

ClasherKasten commented 1 year ago

Can you tell me what functions should I unittest because in testMate I am testing isCheckmate?

Here is a little definition:

A unit test should have no dependencies on code outside the unit tested. You decide what the unit is by looking for the smallest testable part. Where there are dependencies they should be replaced by false objects. StackOverflow

I think there is a way to isolate the checkmate behavior much better from the rest of the program. And if that's not something you want to do right now, there are enough other parts of the code that need to be tested as well. As an example, Coordinate is completely isolated. Most of the types of pieces can be tested when the board is mocked (or the test for the board is also pushed, or if you have another solution just propose it).

What I'd like to say in conclusion is that it's fine if you want to verify that it actually analyzes, moves and recognizes everything (the whole process and how each small part is put together). But then you shouldn't do it just for checkmate. Just checking that it validates a specific example is not the idea of tests. Tests should cover as many possibilities as possible (which includes edge cases). And currently I don't really see this code covering anything (especially not edge cases).

So if you wanna push tests that look like the one you pushed, keep going. but then keep in mind that you also have to test input, output, behaviour, etc. And don't forget edge cases (invalid input, etc.). Also if you only test set input, then you never test that the AI actually works.

GrbavaCigla commented 1 year ago

Yeah, added more tests, but it isn't easy writing 'isolated' tests while everything is entangled in code.

ClasherKasten commented 1 year ago

Yeah, added more tests, but it isn't easy writing 'isolated' tests while everything is entangled in code.

That's a valid point. So you identified that the problem is in the code. A solution to this could also be to discuss how the code could be changed to be more isolated.

But I don't wanna be so negative. Instead: the new tests look very good and seem to improve coverage quite a bit. 👍 Still I would like an answer if you wanna push more or if I can merge this PR and new ones can be opened for more tests?

GrbavaCigla commented 1 year ago

You can merge it :D