Get tests running cleanly again

[mrchrisadams]

• [x] Work out how to make a fixture for when we look up a domain, to resolve to one of our green ip fixtures
• [x] Work out how to get Redis in the test suite (Predis or vanilla php Redis library?)

Incomplete tests

AS tests

• [x] Models_SitecheckAsTest::testDoubleAsNumbersInAsOutputShouldReturnGreen - This website has moved hosts, so we need a different fixture
• [x] Models_SitecheckAsTest::testGetAsForurlForIpv6 - This website has moved hosts, so we need a different fixture
• [x] Models_SitecheckAsTest::testGetSmallestAsForIpv4 - This URL ias associated with a different AS now, so we need a different fixture

Caching tests

• [x] Models_SitecheckCachingTest::testSecondCheckShouldBeCached -This test needs Redis added back in the stack
• [x] Models_SitecheckCachingTest::testSecondCheckWhenCachedShouldBeLogged - This test needs Redis added back in the stack before can check caching behaviour
• [x] Models_SitecheckCachingTest::testResultCachingCanBeReset This test needs Redis added back in the stack before can check caching behaviour
• [x] Models_SitecheckCachingTest::testHostnameLookupsShouldComeFromCacheSecondTime This test needs Redis added back in the stack before can check caching behaviour
• [x] Models_SitecheckCachingTest::testAsLookupsShouldComeFromCacheSecondTime - This test needs Redis added back in the stack before can check caching behaviour
• [x] Models_SitecheckCachingTest::testIpChecksInDatabaseShouldComeFromCacheSecondTime - This test needs Redis added back in the stack before can check caching behaviour
• [x] Models_SitecheckCachingTest::testCacheCanBeSet - This test needs Redis added back in the stack before can check caching behaviour

SiteCheck tests

• [x] Models_SitecheckTest::testCheckingAValidFullUrlShouldReturnTrue - This test needs Redis added back in the stack before can check caching behaviour
• [x] Models_SitecheckTest::testUrlInIpSearchShouldReturnSearchResult -We do not seem to have a checkIp function on the greencheckIP class, which is what the the sitecheck calls in $this->sitecheck->checkIp
• [x] Models_SitecheckTest::testWebsiteHostedByGreenHostingProviderShouldReturnIdOfHostingProvider - We do not have a site with a green provider in the fixtures to check against. Do we need to fake a dns resolution to a GreenIP for a hoster here?
• [x] Models_SitecheckTest::testWebsiteHostedByAnotherGreenHostingProviderShouldReturnIdOfHostingProvider - result returns as null here too. ping can not find it either now. dead domain?
• [x] Models_SitecheckTest::testReturnTheSmallestIpRangePossible - This is calls checkIp on sitecheck, which calls a getIpForUrl, which is returning a null result
• [x] Models_SitecheckTest::testGetIpv6ForurlForIpv6 - This website has moved hosts, so we need a different fixture
• [x] Models_SitecheckTest::testUrlWithIpv6ShouldReturnValidResult - This website has moved hosts, so we need a different fixture
• [x] Models_SitecheckTest::testResultForUrlWithIpv6IpAdressShouldReturnGreen - This website has moved hosts and no longer green. We need a different fixture

Validation checks

• [x] Models_SitecheckValidationTest::testValidatingAValidIpv6AdressShouldReturnTrue - This website has moved hosts, so we need a different fixture

[mrchrisadams]

Getting Redis working in tests

Okay, after checking with @arendjantetteroo we'll need an extension to make \Redis available in the root namespace.

So something like this in the steps to set up our php runtime:

pecl install redis

There will be nicer ways to get it running, later.

[mrchrisadams]

Getting the AS tests sorted

We're using bits of Symfony, so it looks like we can mock out dns checks, to make domains, resolve to green IPs and a green AS. https://symfony.com/blog/new-in-symfony-3-1-network-mocking-and-dns-sensitive-tests

[arendjantetteroo]

The mocking of dns has some issues in that it only works on the second time a dns_get_record is called, the first time it still does a network call to get the real data. Quite possibly this is because of the php bug : https://bugs.php.net/bug.php?id=64346

Anyway it shows that their is a design issue in the code that we need to mock this function for all tests we run.

It would be best to mock the DnsFetcher class so it just returns ip addresses we need in the SitecheckTest and then add a seperate test class just for the DnsFetcher to test the ipv4/6 behaviour, which we can then setup correclty with the symfony or php-mock behaviour.

[mrchrisadams]

Thanks @arendjantetteroo

Do you have a preference on the library for mocking?

I see php unit has test doubles, and we aren't calling any static or other kinds of functions that would stop it working (notes).

[mrchrisadams]

Oh hang, on I see the example in SitecheckTest is already using PHPMock. I'll follow that pattern

[arendjantetteroo]

no preference, whatever feels most natural to you :)

[mrchrisadams]

Hey @arendjantetteroo I can't tell what I'm doing wrong with mocking these tests here.

I expected to be able to mock out a function on the DNS fetcher class like so, because we're calling it in later on inside getHostByName on SiteCheck:

$dns = $this->getFunctionMock(
  'TGWF\Greencheck\Sitecheck\DnsFetcher', "getIpAddressesForUrl"
);
$dns->expects($this->once())->with('http://www.iping.nl/en/test')->willReturn(
      // make the return value match the signature in DNSFetcher, and match a green url
      [
          'ip' => '94.75.237.71',
          'ipv6' => false
      ]
);

And I've run this before a SiteCheck is created, and I've tried running it after a check is created, inside a test function.

Neither seems to change the return value here.

How should you be overriding the behaviour in a mock, if you want to return these values?

[arendjantetteroo]

That would work except for the fact that the DnsFetcher is not injected but created in the Sitecheck class right now. So it doesn't know that it should use the mock you just created.

[mrchrisadams]

Ah... OK.

So to use the mocks we'd need to change the signature of the constructor for sitecheck, or mock the method on Sitecheck that calls into DnsCheck before we instantiate it?

Sent from my phone. Please excuse typos and brevity. See all the ways to contact me at https://productscience.co.uk

On Thu, 18 Apr 2019, 20:07 Arend-Jan Tetteroo, notifications@github.com wrote:

That would work except for the fact that the DnsFetcher is not injected but created in the Sitecheck class right now. So it doesn't know that it should use the mock you just created.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/thegreenwebfoundation/thegreenwebfoundation/pull/10#issuecomment-484620625, or mute the thread https://github.com/notifications/unsubscribe-auth/AAAEL4T3INXFPZNLG7SMSC3PRC2GJANCNFSM4HGVUAGA .

[arendjantetteroo]

Yep, see my new commit of how that works.

Now you would need to change the mock in a couple tests to output something you would need in that specific test. Or change the used ip address, but you probably need a couple to test the different pathways (ip, url, as all probably need different ip's)

[arendjantetteroo]

check this example for how to properly set it up for multiple results: https://phpunit.de/manual/6.5/en/test-doubles.html#test-doubles.stubs.examples.StubTest5.php

[arendjantetteroo]

I added a commit with example. It now has 16 errors, but if you add the proper mapping to the $map it will all be running flawlessly. Just check the line that goes wrong in sitecheckTest and then add the proper value to $map.

[arendjantetteroo]

down to 10, the rest i'll leave to you

[mrchrisadams]

Hey @arendjantetteroo thanks for the review - I'll push these changes shortly

[arendjantetteroo]

Code updated for the review. looks good to me.

[mrchrisadams]

Hey @arendjantetteroo how do you feel about thus merging in?

Worth rewriting the history to clean it up, or are you happy to just rebase and merge?

[arendjantetteroo]

@mrchrisadams i would squash it, lots of really small or trying commits that don't bring much value in the commit history.