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

initial changes to support unit testing via phpunit #911

Merged
merged 19 commits into from
Nov 12, 2020

Conversation

XaeroDegreaz
Copy link
Collaborator

@XaeroDegreaz XaeroDegreaz commented Nov 4, 2020

There is a simple test for retrieving accounts via account id using a mock mysql object

closes #910

…t for retrieving accounts via account id using a mock mysql object
composer.json Outdated Show resolved Hide resolved
@hemberger
Copy link
Member

I'm coming around to your idea of parsing .env instead of having a separate mysql secrets file. Would something like https://github.com/vlucas/phpdotenv allow us to avoid the override logic in SmrMySqlDatabase?

.travis.yml Outdated Show resolved Hide resolved
@XaeroDegreaz
Copy link
Collaborator Author

I'm coming around to your idea of parsing .env instead of having a separate mysql secrets file. Would something like https://github.com/vlucas/phpdotenv allow us to avoid the override logic in SmrMySqlDatabase?

Yeah I think you could make that work. Like we discussed earlier, I would just make sure that it doesn't actually read them into the environment for security.

@XaeroDegreaz XaeroDegreaz marked this pull request as ready for review November 11, 2020 00:43
@XaeroDegreaz XaeroDegreaz changed the title [WIP] initial changes to support unit testing via phpunit initial changes to support unit testing via phpunit Nov 11, 2020
@hemberger
Copy link
Member

I see some Scrutinizer warnings along the lines of:

test/SmrTest/lib/DefaultGame/AbstractSmrAccountIntegrationTest.php#L167
$socialLogin of type Mockery\Mock is incompatible with the type SocialLogin expected by parameter $social of AbstractSmrAccount::getAccountBySocialLogin().

Obviously we don't want to compromise our method typehints for the sake of the test infrastructure, but is there a recommended way to address this sort of issue, or is a linter like Scrutinizer always going to have a problem with test code like this?

Some quick googling shows discussions about issues that look similar to me, but it seems like it requires a nuanced understanding of PHPUnit, which I currently don't have!

@XaeroDegreaz
Copy link
Collaborator Author

XaeroDegreaz commented Nov 11, 2020

Let me look into this -- The object passed is not strictly of the type being mocked, but rather a proxy. I don't think that it compromises the typehint integrity in such a way that would affect production code, but it is a little annoying to see that the types returned from Mockery are not statically typed. However, to be fair, PHP is not a statically typed language, so this sort of makes sense.

Standard PHPUnit mock methods are "typed" using @psalm-* annotations that help return correct type hints. However, we're using Mockery mocks. Initially this was for its ability to produce global instance mocks of hard dependencies. However, since I've abandoned mocking hard dependencies in favour of just doing proper integration tests, perhaps I can remove Mockery as a dependency, and use the PHPUnit mocks instead. I will look into what sort of mock interaction verification the framework has, as that's a really important part of testing.

…hould get rid of any type hinting warnings from scrutinizer.
@XaeroDegreaz
Copy link
Collaborator Author

Ok, I've updated the tests to use the PHPUnit mocks instead. Their typehinting annotations should be good (at least the errors are gone from my IDE).

@hemberger hemberger merged commit 0710abf into smrealms:master Nov 12, 2020
@XaeroDegreaz XaeroDegreaz deleted the 910-account-test-coverage branch November 12, 2020 00:19
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.

Increase test coverage for AbstractSmrAccount
2 participants