search

moodlehq / moodle-docker

A docker environment for moodle developers
GNU General Public License v3.0
406 stars 252 forks source link

Some tests, e.g. test.html and test.jpg, still required access to the external https://download.moodle.org/unittest #178

Closed scara closed 2 years ago

scara commented 3 years ago

Hello Everybody, on updating my fork I see an unrelated failure on my local GHA run due to a temporary "external issue":

Debugging: cURL request for "https://download.moodle.org/unittest/test.html" failed, HTTP response code: HTTP/2 502 

* line 1682 of /lib/filelib.php: call to debugging()
* line 114 of /lib/tests/filelib_test.php: call to download_file_content()
* line 1526 of /vendor/phpunit/phpunit/src/Framework/TestCase.php: call to core_filelib_testcase->test_download_file_content()
* line 1132 of /vendor/phpunit/phpunit/src/Framework/TestCase.php: call to PHPUnit\Framework\TestCase->runTest()
* line 80 of /lib/phpunit/classes/advanced_testcase.php: call to PHPUnit\Framework\TestCase->runBare()
* line 722 of /vendor/phpunit/phpunit/src/Framework/TestResult.php: call to advanced_testcase->runBare()
* line 884 of /vendor/phpunit/phpunit/src/Framework/TestCase.php: call to PHPUnit\Framework\TestResult->run()
* line 677 of /vendor/phpunit/phpunit/src/Framework/TestSuite.php: call to PHPUnit\Framework\TestCase->run()
* line 677 of /vendor/phpunit/phpunit/src/Framework/TestSuite.php: call to PHPUnit\Framework\TestSuite->run()
* line 677 of /vendor/phpunit/phpunit/src/Framework/TestSuite.php: call to PHPUnit\Framework\TestSuite->run()
* line 667 of /vendor/phpunit/phpunit/src/TextUI/TestRunner.php: call to PHPUnit\Framework\TestSuite->run()
* line 143 of /vendor/phpunit/phpunit/src/TextUI/Command.php: call to PHPUnit\TextUI\TestRunner->run()
* line 96 of /vendor/phpunit/phpunit/src/TextUI/Command.php: call to PHPUnit\TextUI\Command->run()
* line 61 of /vendor/phpunit/phpunit/phpunit: call to PHPUnit\TextUI\Command::main()
...
Time: 56:56.097, Memory: 1.03 GB

There was 1 failure:

1) core_filelib_testcase::test_download_file_content
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-'47250a973d1b88d9445f94db4ef2c97a'
+'d41d8cd98f00b204e9800998ecf8427e'

/var/www/html/lib/tests/filelib_test.php:115
/var/www/html/lib/phpunit/classes/advanced_testcase.php:80

which is unexpected to me since I'd guess exttests should be targeted even here, in the local tests.

It appears that "a family of tests" in Moodle core code, based on /lib/phpunit/classes/advanced_testcase.php::getExternalTestFileUrl(), has been left out from the scope of exttests, probably due to the unavailability of a valid HTTPS certificate and the properly set of TEST_EXTERNAL_FILES_HTTPS_URL.

Should we, when running here the tests, define TEST_EXTERNAL_FILES_HTTPS_URL as false to skip the above tests, avoiding to overload the Moodle Download site?

Note: I still miss why getExternalTestFileUrl() returns the HTTPS URL, since the local Moodle instance is exposed via HTTP and the failed test do not explicitly require HTTPS.

HTH, Matteo

stronk7 commented 3 years ago

Hi,

  1. I think that the https ones can be put to http just specifying it also in config. For example, @ [moodle-ci-runner(https://github.com/moodlehq/moodle-ci-runner) we have both variables set to http (link). And it works perfectly. So I think that we can do pretty much the same here, in moodle-docker.
if (!empty(getenv('EXTTESTURL'))) {
    define('TEST_EXTERNAL_FILES_HTTP_URL', getenv('EXTTESTURL'));
    define('TEST_EXTERNAL_FILES_HTTPS_URL', getenv('EXTTESTURL'));
}

  1. That said, yeah, we must ensure that there aren't any other calls to downloads (haven't looked) and fix all them.

  2. Finally, curiously, this called my attention because, while I was playing to get multi-arch php moodlehq images (amd64 and arm64, yay, it seems to be working great), I've been performing a number or complete phpunit runs here (both architectures). And I've been getting a consistent failure with a test that downloads user.jpg (from memory), always with a CURL timeout error. But, the URL was the correct one (http://exttests/user.jpg), so I've to dig into that problem. It seems that other runs aren't affected, it only happens in my local docker instances (amd64 and arm64).

So, yeah, we should sort out 1 & 2. I'm going to start a run here without networking to see which tests are, effectively, trying to access outside (I imagine that there will be some H5P-related ones, but there shouldn't be others, agree).

stronk7 commented 3 years ago

Here it's a complete run: https://pastebin.com/2fBj8czB

With only change performed being:

--- a/config.docker-template.php
+++ b/config.docker-template.php
@@ -47,6 +47,7 @@ $CFG->pathtophp = '/usr/local/bin/php';
 $CFG->phpunit_dataroot  = '/var/www/phpunitdata';
 $CFG->phpunit_prefix = 't_';
 define('TEST_EXTERNAL_FILES_HTTP_URL', 'http://exttests:9000');
+define('TEST_EXTERNAL_FILES_HTTPS_URL', 'http://exttests:9000');

 $CFG->behat_wwwroot   = 'http://webserver';
 $CFG->behat_dataroot  = '/var/www/behatdata';

And this is the list of problems (grouped by type) while it was run with networking disabled:

Basically all the oauth ones are about resolving some host, and the antivirus ones about (surprisingly) performing ip-lookups. Nothing related with exttests, so far.

There is also a failure that I'm getting locally all the time since some days ago and cannot reproduce @ CIs, but that's another story, heh.

So, maybe, all we need is to add TEST_EXTERNAL_FILES_HTTPS_URL and done.

scara commented 3 years ago

Hi @stronk7, thanks a ton for your deep diving! 👍

My HTTPS failure was related to https://github.com/moodle/moodle/blob/0d0e66d37c6271657d76f948db9eeb10c139e7ae/lib/tests/filelib_test.php#L111-L115 so it doesn't hurt even there.

+1 to your proposal to mimic what already in place at https://github.com/moodlehq/moodle-ci-runner.

Unfortunately, we can define and distribute the $CFG->geoip2file file to avoid calling http://www.geoplugin.net/json.gp?ip=<IP Address>.

TIA, Matteo