search

buttonmen-dev / buttonmen

Buttonmen - an online dice game
Other
16 stars 24 forks source link

Site upgrade #2398

Closed blackshadowshade closed 6 years ago

blackshadowshade commented 6 years ago

The production instance that we are currently running needs updating, for a number of reasons:

Specific issues that may relate to this site upgrade are:

blackshadowshade commented 6 years ago

So, I see a good endpoint here as:

blackshadowshade commented 6 years ago

One desirable endpoint is that we develop a procedure for doing such site upgrades, so that we can do them as necessary in the future.

cgolubi1 commented 6 years ago

Regarding:

It sounds like what you're really looking for here is an audit --- you want someone to look at every piece of software we're running, propose a current version, and make a plan for upgrading to that current version.

But you're still not really saying why. I'm seeing this as:

And, okay, if step 1 is to do an audit, i'm happy to do an audit as step 1. But i want to be able to say stuff like: "We're running MySQL X, the newest MySQL is Y; MySQL X is still supported, so there's nothing to do here," and i think the onus should be on someone to say there's an actual benefit to upgrading beyond "Y is greater than X and big numbers are better."

Note that if version X is no longer getting minor revision security updates, fine, that is a reason. But most major software continues to support old versions.

What i'm saying here is not so much an argument with anything, as a notice of how i would intend to proceed with this. So if you have specifics like "We need PHP Y because PHP X is messing us up in the following way," the most valuable thing you can do is put specific information about why this issue is needed, on this issue.

blackshadowshade commented 6 years ago

Here are the current problems that we are currently experiencing (off the top of my head), due to the old versioning of our software products:

cgolubi1 commented 6 years ago

Okay, so, one note is that i'm not planning to integrate R as part of this upgrade --- i understand that wanting to add R is one of your reasons for wanting the upgrade to get done, but it is a separate thing. I think the two major asks here are:

So i'm going to call those things the acceptance criteria for this issue (and i agree that it almost certainly makes sense to do them together rather than separately, because either of debugging old Ubuntu + new PHP or old PHP + new Ubuntu is likely to be more hassle than it's worth, though i'll probably take a few minutes to test that theory).

blackshadowshade commented 6 years ago

Okay, that sounds fine. I've tested on stats.dev that Xenial is okay for running R, so anything at least that recent will work for a subsequent installation of R.

cgolubi1 commented 6 years ago

I have a virtualbox xenial instance that was able to get far enough to run phpunit. Here's the phpunit output so far --- it's possible that there are simple "tolerate X" type flags whose defaults have changed between php5 and php7, that we could modify. But i figured i'd throw these out here because some of them may simply be code shortcuts that would be easy for us to fix. I'll work on them myself under that assumption as time permits, but Shadowshade you may be interested in looking yourself:

PHPUnit 5.1.3 by Sebastian Bergmann and contributors.

...................ICaught exception in BMInterface::submit_die_values: Undefined offset: 2
....I...........II.........................  63 / 744 (  8%)
............................................................... 126 / 744 ( 16%)
............................................................... 189 / 744 ( 25%)
............................................................... 252 / 744 ( 33%)
.....................................................Caught exception in BMDie::create_from_string_components: Invalid recipe: himom!
Caught exception in BMDie::create_from_string_components: Invalid recipe: 75.3
Caught exception in BMDie::create_from_string_components: Invalid recipe: trombones76
Caught exception in BMDie::create_from_string_components: Invalid recipe: 76trombones
.......... 315 / 744 ( 42%)
.................III........................................... 378 / 744 ( 50%)
............................................................... 441 / 744 ( 59%)
..........................................I.......I............ 504 / 744 ( 67%)
.............E....E.................F.........................F 567 / 744 ( 76%)
...........E.................F...........E............F........ 630 / 744 ( 84%)
....F.......................................................... 693 / 744 ( 93%)
...................................................             744 / 744 (100%)

Time: 1.19 minutes, Memory: 30.00Mb

There were 4 errors:

1) BMGameTest::test__set_active_die_array_array
TypeError: Argument 2 passed to BMGame::setBMPlayerProps() must be of the type array, string given, called in /buttonmen/src/engine/BMGame.php on line 3288

/buttonmen/src/engine/BMGame.php:4537
/buttonmen/src/engine/BMGame.php:3288
/buttonmen/test/src/engine/BMGameTest.php:2552

2) BMGameTest::test__set_captured_die_array_array
TypeError: Argument 2 passed to BMGame::setBMPlayerProps() must be of the type array, string given, called in /buttonmen/src/engine/BMGame.php on line 3288

/buttonmen/src/engine/BMGame.php:4537
/buttonmen/src/engine/BMGame.php:3288
/buttonmen/test/src/engine/BMGameTest.php:2730

3) BMGameTest::test_default_round
Parameter 1 to end() expected to be a reference, value given

/buttonmen/test/src/engine/BMGameTest.php:9986

4) BMInterfaceGameTest::test_react_to_auxiliary_invalid
TypeError: Argument 1 passed to BMInterfaceGame::is_action_current() must be an instance of BMGame, boolean given, called in /buttonmen/src/engine/BMInterfaceGame.php on line 1043

/buttonmen/src/engine/BMInterfaceGame.php:1581
/buttonmen/src/engine/BMInterfaceGame.php:1043
/buttonmen/test/src/engine/BMInterfaceGameTest.php:1056

--

There were 5 failures:

1) BMGameTest::test_berserk_round
Failed asserting that false is true.

/buttonmen/test/src/engine/BMGameTest.php:5877

2) BMGameTest::test_option_game
Failed asserting that false is true.

/buttonmen/test/src/engine/BMGameTest.php:9027

3) BMInterfaceGameTest::test_create_game_with_two_random_buttons
Failed asserting that true is false.

/buttonmen/test/src/engine/BMInterfaceGameTest.php:437

4) BMInterfaceTest::test_swing_value_reset_at_end_of_round
Failed asserting that false is true.

/buttonmen/test/src/engine/BMInterfaceTest.php:412

5) BMInterfaceTest::test_option_game
Failed asserting that false is true.

/buttonmen/test/src/engine/BMInterfaceTest.php:1416

FAILURES!
Tests: 744, Assertions: 13754, Errors: 4, Failures: 5, Incomplete: 9.
cgolubi1 commented 6 years ago

The first two errors are easily fixed by having the catch expect TypeError rather than PHPUnit_Framework_Error, so i'm on that. The third one is in this code:

        // james: note that end normally shifts the pointer, so use
        // call_user_func and array_values to avoid changing the original array
        $lastLogEntry = call_user_func('end', array_values($game->actionLog));

so i am definitely going to rely on Shadowshade to reverse-engineer what we were trying to accomplish there.

blackshadowshade commented 6 years ago

Okay, here we go. In PHP 5.3.0, using call_user_func with a non-reference call when a reference call is expected now creates an E_WARNING (see http://php.net/manual/en/function.call-user-func.php).

Looking at the code again (and reading https://stackoverflow.com/questions/3687358/whats-the-best-way-to-get-the-last-element-of-an-array-without-deleting-it), we'd probably both prefer replacing that nasty line and the associated comment (all three times that they appear) with

$lastLogEntry = $game->actionLog[count($game->actionLog) - 1];
cgolubi1 commented 6 years ago

Sounds reasonable. I'll try it.

cgolubi1 commented 6 years ago

That seems to have helped. Here's the current state:

sandbox,[/buttonmen/src],13:20(130)$ sudo phpunit --bootstrap /usr/local/etc/buttonmen_phpunit.php /buttonmen/test/src/engine/BMInterfaceGameTest.php
PHPUnit 5.1.3 by Sebastian Bergmann and contributors.
...E............F............F................................. 567 / 643 ( 88%)
............................................................... 630 / 643 ( 97%)
.............                                                   643 / 643 (100%)

Time: 16.49 seconds, Memory: 24.00Mb

There was 1 error:

1) BMInterfaceGameTest::test_react_to_auxiliary_invalid
TypeError: Argument 1 passed to BMInterfaceGame::is_action_current() must be an instance of BMGame, boolean given, called in /buttonmen/src/engine/BMInterfaceGame.php on line 1043

/buttonmen/src/engine/BMInterfaceGame.php:1581
/buttonmen/src/engine/BMInterfaceGame.php:1043
/buttonmen/test/src/engine/BMInterfaceGameTest.php:1056

--

There were 5 failures:

1) BMGameTest::test_berserk_round
Failed asserting that false is true.

/buttonmen/test/src/engine/BMGameTest.php:5877

2) BMGameTest::test_option_game
Failed asserting that false is true.

/buttonmen/test/src/engine/BMGameTest.php:9027

3) BMInterfaceGameTest::test_create_game_with_two_random_buttons
Failed asserting that true is false.

/buttonmen/test/src/engine/BMInterfaceGameTest.php:437

4) BMInterfaceTest::test_swing_value_reset_at_end_of_round
Failed asserting that false is true.

/buttonmen/test/src/engine/BMInterfaceTest.php:412

5) BMInterfaceTest::test_option_game
Failed asserting that false is true.

/buttonmen/test/src/engine/BMInterfaceTest.php:1416

FAILURES!
Tests: 643, Assertions: 9864, Errors: 1, Failures: 5, Incomplete: 5.
sandbox,[/buttonmen/src],13:23(2)$
cgolubi1 commented 6 years ago

The following change to BMInterfaceGame fixes the remaining error, and IMO is a good idea anyway:

$ git diff src/engine/BMInterfaceGame.php 
diff --git a/src/engine/BMInterfaceGame.php b/src/engine/BMInterfaceGame.php
index 340c32b..f50ad38 100644
--- a/src/engine/BMInterfaceGame.php
+++ b/src/engine/BMInterfaceGame.php
@@ -1035,6 +1035,10 @@ class BMInterfaceGame extends BMInterface {
     ) {
         try {
             $game = $this->load_game($gameId);
+            if (!$game) {
+                $this->set_message('Invalid game');
+                return FALSE;
+            }
             if (!$this->is_action_current(
                 $game,
                 BMGameState::CHOOSE_AUXILIARY_DICE,
leocrotta,[~/src/git/cgolubi1/buttonmen-2398_site_upgrade],09:28(0)$
blackshadowshade commented 6 years ago

Yep, that looks like a good idea.

cgolubi1 commented 6 years ago

Hey, check this out --- after months (possibly hours) of toil, i have gotten circleci to fail similarly to how my local phpunit is failing. In order to get circleci to run under xenial, i had to switch us from the "machine" executor to the "docker" executor. To my knowledge, there's no downside to the docker executor except that i didn't want to debug getting it working, but now that i've done so, unless we hit any problems in the post-phpunit steps, we should be good to just use it from now on.

https://circleci.com/gh/cgolubi1/buttonmen/143

cgolubi1 commented 6 years ago

I stood up a dev site for this at 54.159.124.54 --- irilyth, mind setting that up as "xenial"? (If i get a chance to setup my own route53-updater, i will, but one thing at a time, so if you have it handy, go for it.)

cgolubi1 commented 6 years ago

Shadowshade: if you want to see the phpunit failures interactively, you can login to that site and run:

sudo /usr/local/bin/run_buttonmen_tests

In theory you can also pull from my branch and setup your own local test site under vagrant/virtualbox, of course.

cgolubi1 commented 6 years ago

Other testers, if you want to check the dev site out and play some games, that would be great. My working assumption is that unit tests are failing because shadowshade took shortcuts when setting up objects for some of the tests, and php7 is stricter in ways that outlaw some of those shortcuts. That is, the problem is in the tests, not in the code. So far, that's proved true in all the tests we've root-caused. Anything weird people see in gameplay or the UI should be reported.

cgolubi1 commented 6 years ago

By the way, i should mention that (a) i'm currently optimistic about our ability to get this working pretty soon, as a result of which (b) i think we should stay in merge-freeze mode until we either get this over the fence or conclude it's not going to be that easy. Given that we expect to do this as a site-rebuild/cutover with downtime, it'll simplify matters considerably if we're not trying to apply any database changes at the same time. So i think we should ignore the release schedule in the interest of getting this one change ready to go and going as soon as we think we're ready.

cgolubi1 commented 6 years ago

This change makes test_berserk_round() pass:

$ git diff
diff --git a/test/src/engine/BMGameTest.php b/test/src/engine/BMGameTest.php
index 5b08146..45c0963 100644
--- a/test/src/engine/BMGameTest.php
+++ b/test/src/engine/BMGameTest.php
@@ -5874,7 +5874,7 @@ class BMGameTest extends PHPUnit_Framework_TestCase {
         $this->assertEquals('V', $game->activeDieArrayArray[1][2]->swingType);
         $this->assertEquals(11, $game->activeDieArrayArray[1][2]->swingValue);
         $this->assertEquals(6, $game->activeDieArrayArray[1][2]->max);
-        $this->assertTrue(isset($game->activeDieArrayArray[1][2]->value));
+        $this->assertNotNull($game->activeDieArrayArray[1][2]->value);

         $this->assertEquals(array(40.5, -12.0), $game->roundScoreArray);
     }

and when i modify the assert so it lets me look at the actual value, it's some number like 1, or 3, or 4 --- it's the value of a die which was rerolled, like you'd expect.

So i'm guessing the behavior of isset() changed. http://php.net/manual/en/function.isset.php suggests cryptically:

Version   Description
5.4.0     Checking non-numeric offsets of strings now returns FALSE.

I guess i have a couple of comments about this:

cgolubi1 commented 6 years ago

My shot in the dark was wrong. If i modify the test in the location we're looking at to include:

        $this->assertEquals(6, $game->activeDieArrayArray[1][2]->max);
        $this->assertTrue(isset($game->activeDieArrayArray[1][2]->max));

the test fails on the second line. So max is equal to 6, but it's not "set". My takeaway is that my modification to the test is fine, and we should look around for places where we use isset() on members of die objects in the actual code. And do a bunch of replay testing.

[Incidentally, my new shot-in-the-dark theory is that this is related to the fact that:

        error_log(var_export($game->activeDieArrayArray[1][2], $return=TRUE));

fails with:

var_export does not handle circular references

There's something structurally suspect about the BMDie object, and i don't think that's new (i've noticed the var_export thing before). If we can get through this upgrade without debugging it, so much the better as far as i'm concerned.]

cgolubi1 commented 6 years ago

test_option_game has the same issue (isset() failing on members of die objects, replacing with assertNotNull() works).

cgolubi1 commented 6 years ago

Yeah, so this has got to be a result of the changes to the behavior of isset() (and the related empty(), which is used in at least one of the other tests i've found so far). The documentation pages http://php.net/manual/en/function.isset.php and http://php.net/manual/en/function.empty.php both mention that change in 5.4.

There's a comment on the former page which seems instructive "...will always echo 'false'. because the isset() accepts VARIABLES as it parameters, but in this case, $foo->bar is NOT a VARIABLE. it is a VALUE returned from the __get() method of the class Foo. thus the isset($foo->bar) expreesion will always equal 'false'."

And we can trivially verify that this is the issue (WARNING: when you read this next bit, you will involuntarily say "oh my god WHY are we using PHP?" out loud, so make sure that's okay with your neighbors). This code?

        $myvar = $game->buttonArray[0]->name;
        $this->assertFalse(empty($myvar));
        $this->assertFalse(empty($game->buttonArray[0]->name));

Guess which line that fails on? Yep. The last one.

TL;DR: let's see how hard it would be to remove isset() and empty() from all of our code because they are completely terrifying (and, more to the point, because the behavior changes in 5.4 are just not going to work well with our data structures).

blackshadowshade commented 6 years ago

Argh. That sounds like more than a trivial amount of work.

I'll take a look when I can.

cgolubi1 commented 6 years ago

...yeah. I was going to say it's not that bad, but in fact it sort of is that bad. Looking at non-test code:

leocrotta,[~/src/git/cgolubi1/buttonmen-2398_site_upgrade/src],18:59(0)$ find . -type f -name '*.php' -print0 | xargs -0 grep -I 'isset(' | wc     226     982   16381
leocrotta,[~/src/git/cgolubi1/buttonmen-2398_site_upgrade/src],18:59(0)$ find . -type f -name '*.php' -print0 | xargs -0 grep -I 'empty(' | wc 
      98     399    6866

As far as i can tell, item 10 at https://www.toptal.com/php/10-most-common-mistakes-php-programmers-make seems pretty relevant to us. (And i don't see why it's not also an issue with isset().)

Anyway, before you go off and boil the ocean, we should talk on IRC about whether boiling the ocean is actually necessary --- i can think of a couple of options that might be less painful than replacing all occurrences of these two functions one by one.

cgolubi1 commented 6 years ago

Also potentially relevant --- https://stackoverflow.com/questions/21227585/what-is-the-difference-between-isset-and-isset

Presumably we knew something about __isset() at some point, because we defined it in a bunch of classes. Anyway, i'm going to make one more comment summarizing the current state, and then i'm going to say the ball's in your court, and let's talk.

cgolubi1 commented 6 years ago

Oh, quick summary of the issues i'm having with phantomjs, because i'm not going to run them to ground right now:

We are impacted by https://github.com/ariya/phantomjs/issues/14376 and also https://github.com/emberjs/ember.js/issues/13331 the upshot of both of which seems to be that we should have circleci download https://bitbucket.org/ariya/phantomjs/downloads/phantomjs-1.9.8-linux-x86_64.tar.bz2 and use that phantomjs binary rather than try to use the Ubuntu phantomjs (if we only had to solve the first issue, there are some workarounds --- i had some success with both QT_QPA_PLATFORM=minimal and xvfb-run in hand-testing --- but given that the second issue wants us to downgrade, might as well downgrade and solve both problems).

Currently, the test is failing with:

# /root/phantomjs-1.9.8-linux-x86_64/bin/phantomjs --web-security=false /usr/local/etc/run-jscover-qunit.js http://localhost/test-ui/phantom-index.html
'waitFor()' timeout
Unsafe JavaScript attempt to access frame with URL about:blank from frame with URL file:///usr/local/etc/run-jscover-qunit.js. Domains, protocols and ports must match.

Unsafe JavaScript attempt to access frame with URL about:blank from frame with URL file:///usr/local/etc/run-jscover-qunit.js. Domains, protocols and ports must match.

Unsafe JavaScript attempt to access frame with URL about:blank from frame with URL file:///usr/local/etc/run-jscover-qunit.js. Domains, protocols and ports must match.

Unsafe JavaScript attempt to access frame with URL about:blank from frame with URL file:///usr/local/etc/run-jscover-qunit.js. Domains, protocols and ports must match.

I have not started looking into what's causing that.

Ah, never mind --- it's https://github.com/ariya/phantomjs/issues/12697 --- 1.9.8 doesn't honor --web-security=false.

However:

root@ab16522ac5dc:~/project# /root/phantomjs-1.9.7-linux-x86_64/bin/phantomjs --web-security=false /usr/local/etc/run-jscover-qunit.js http://localhost/test-ui/phantom-index.html
'waitFor()' timeout
root@ab16522ac5dc:~/project# echo $?
1
root@ab16522ac5dc:~/project#

So i'm not done yet.

cgolubi1 commented 6 years ago

Current status of Ubuntu/PHP upgrade:

Working:

Not working/still to do:

irilyth commented 6 years ago

I stood up a dev site for this at 54.159.124.54 --- irilyth, mind setting that up as "xenial"?

Done!

cgolubi1 commented 6 years ago

I got a replay loop running for my branch; let's see how that goes.

cgolubi1 commented 6 years ago

For those playing the home game, the replay site has now played 3600 each new and replayed games without blowing up. So that implies that if there's a regression related to these values, it's something pretty subtle. Obviously i want to see a lot more replay games before i declare that this is fine, but it's adding 1000/day, so the passage of time should get us there.

My inclination given that is to say that it's more dangerous than helpful to try to change hundreds of LOC to get rid of isset() and empty(), and that if we're happy with the test code changes in my branch, we should call that good enough. Shadowshade, you have more of a stake than i do in exactly what all of these detailed tests are doing --- how uncomfortable does that idea make you, and do you have a proposal at this time for what we should do about it?

blackshadowshade commented 6 years ago

As far as I understand, any uses of isset() and empty() that occur within a class are perfectly fine, since these would bypass the magic methods.

I was going to go through the code and check for instances of isset() and empty() that reference different classes, and see how many of those I found. However, I'm still playing catch-up at the moment from being really busy, so I haven't started this yet.

I'm glad that replay testing is passing, although I'm not entirely surprised, considering that I've been able to manually play games under PHP 7 on my stats.dev instance with the changes that I put through in #2338.

I've spent the last hour trying to replicate the test failures in my MAMP stack, but am not there yet (especially since PHPUnit 6 and NetBeans 8.2 aren't compatible at all). I'd suggest that this is where I should be spending my time, so that I'm seeing identical or similar behaviour to the new xenial instance. By the time I'm able to reproduce the failures, your replay rig should be able to give us the final word on whether there are likely to be any subtle errors still there.

cgolubi1 commented 6 years ago

Sounds right to me.

cgolubi1 commented 6 years ago

Okay, i finally got a successful CI workflow - https://circleci.com/gh/cgolubi1/buttonmen/164 - so now this is on Shadowshade's plate to get his AMP stack working, and RandomAI's plate to find meaningful logic regressions.

cgolubi1 commented 6 years ago

Hmm, speaking of...

PHPUnit 5.1.3 by Sebastian Bergmann and contributors.

...............................................................  63 / 101 ( 62%)
....................Caught exception in BMInterface::save_game: PDO::prepare(): MySQL server has gone away

Why would that happen?

cgolubi1 commented 6 years ago

It looks from the log like a quasi-normal restart:

2018-10-24T06:06:31.545246Z 0 [Note] Giving 2 client threads a chance to die gracefully
2018-10-24T06:06:31.545952Z 0 [Note] Shutting down slave threads
2018-10-24T06:06:33.560327Z 0 [Note] Forcefully disconnecting 0 remaining clients
2018-10-24T06:06:33.561118Z 0 [Note] Event Scheduler: Purging the queue. 0 events
2018-10-24T06:06:33.564265Z 0 [Note] Binlog end
2018-10-24T06:06:33.571993Z 0 [Note] Shutting down plugin 'auth_socket'
2018-10-24T06:06:33.573183Z 0 [Note] Shutting down plugin 'ngram'
...
2018-10-24T06:07:30.540834Z 0 [Note] Server hostname (bind-address): '127.0.0.1'; port: 3306
2018-10-24T06:07:30.540842Z 0 [Note]   - '127.0.0.1' resolves to '127.0.0.1';
2018-10-24T06:07:30.540868Z 0 [Note] Server socket created on IP: '127.0.0.1'.
2018-10-24T06:07:30.620554Z 0 [Note] Event Scheduler: Loaded 0 events
2018-10-24T06:07:30.620685Z 0 [Note] /usr/sbin/mysqld: ready for connections.
Version: '5.7.24-0ubuntu0.16.04.1'  socket: '/var/run/mysqld/mysqld.sock'  port: 3306  (Ubuntu)

but i don't like mysqld being offline for a full minute.

cgolubi1 commented 6 years ago

This explains it, from /var/log/apt/history.log:

Start-Date: 2018-10-24  06:05:57
Commandline: /usr/bin/unattended-upgrade
Upgrade: mysql-client-5.7:amd64 (5.7.23-0ubuntu0.16.04.1, 5.7.24-0ubuntu0.16.04.1), mysql-server-5.7:amd64 (5.7.23-0ubuntu0.16.04.1, 5.7.24-0ubuntu0.16.04.1), mysql-server:amd64 (5.7.23-0ubuntu0.16.04.1, 5.7.24-0ubuntu0.16.04.1), mysql-client-core-5.7:amd64 (5.7.23-0ubuntu0.16.04.1, 5.7.24-0ubuntu0.16.04.1), mysql-common:amd64 (5.7.23-0ubuntu0.16.04.1, 5.7.24-0ubuntu0.16.04.1), libmysqlclient20:amd64 (5.7.23-0ubuntu0.16.04.1, 5.7.24-0ubuntu0.16.04.1), mysql-server-core-5.7:amd64 (5.7.23-0ubuntu0.16.04.1, 5.7.24-0ubuntu0.16.04.1)
End-Date: 2018-10-24  06:07:32

We can hold the thought of whether this is the behavior we want or not.

For future reference, the cron job which does this is /etc/cron.daily/apt-compat, and per comments in /usr/lib/apt/apt.systemd.daily we could disable it by modifying /etc/apt/apt.conf.d/10periodic with:

#  APT::Periodic::Unattended-Upgrade "0";
#  - Run the "unattended-upgrade" security upgrade script 
#    every n-days (0=disabled)
#    Requires the package "unattended-upgrades" and will write
#    a log in /var/log/unattended-upgrades
blackshadowshade commented 6 years ago

I always knew that AI would take over the world.

blackshadowshade commented 6 years ago

(copied from IRC and slightly formatted for easier comprehension)

So, I'm currently working on getting my PHPUnit tests up and running.

I note that the xenial instance is running PHPUnit 5.1.3. I also note that PHPUnit 5 is already out of support

Add to that the fact that the transition from PHPUnit 5 to PHPUnit 6 requires changing all the

class … extends PHPUnit_Framework_TestCase

to

use PHPUnit\Framework\TestCase;
class MyTestCase extends TestCase

and I ask myself the obvious question: do we want to be using PHPUnit 5 or 6 (noting that PHPUnit 7 doesn't support PHP 7.0 any more)

and the obvious corollary: Is it worth it for me to go to the effort of doing a two-step PHPUnit test upgrade on my AMP stack, or would it be easy enough for us to move the xenial instance up to PHPUnit 6?

blackshadowshade commented 6 years ago

The major justification I have for wanting to discuss this now is that I can't currently run any unit tests under PHPUnit 6, since that would require me to do the 147 replaces in the test code to use the new PHPUnit namespace, and I don't want to have to deal with the mental juggling of toggling these changes in and out as required.

(the clue for what was wrong with my unit tests came from https://stackoverflow.com/questions/29299206/phpunit-no-tests-executed-when-using-configuration-file)

cgolubi1 commented 6 years ago

Ignoring all of the above: why are you running PHPUnit 6? Like, what caused it to happen that when you, shadowshade, ran the tests, you were running them under PHPUnit 6 rather than PHPUnit 5?

cgolubi1 commented 6 years ago

I'm asking because switching away from phpunit5 would be a pretty bad deal for me. phpunit5 is the default version that apt installs under xenial. This means that instead of hackeration to download phpunit, find a way to run it, etc, which has been proven to break multiple times a year because someone in the php world sneezes on something, i can just say:

  package {
    "phpunit": ensure => installed;
  }

and that Does The Right Thing for both virtualbox and circleci/docker.

In addition, we don't have to upgrade those 147 tests.

The fact that the words "out of support" are in some nebulous way attached to phpunit5 has zilch to do with any problems i care about. Upgrading means doing 10 hours of debugging right now to get it running, and having recurring headaches later. If getting phpunit5 installed under AMP is more than a few hours of work for you, let's talk about this. Otherwise, i don't see the benefit, unless i'm missing something.

blackshadowshade commented 6 years ago

I've been attempting to run PHPUnit 6 because that's the version that's currently co-shipping with the version of PHP 7 (7.1.13) under my version of MAMP.

I'm happy to downgrade my installation of PHPUnit to PHPUnit 5 under the understanding that we're not planning to move up to PHPUnit 6 any time soon.

cgolubi1 commented 6 years ago

Yep, exactly --- i don't think we have any reason to be on PHPUnit 6 right now. Give downgrading a shot, and if you have a lot of trouble doing it, complain, and we'll revisit.

blackshadowshade commented 6 years ago

I've successfully downgraded the version of PHPUnit used on my Mac to version 5.7.27 (running with PHP 7.1.13).

When running on the current master, it comes up with failures and errors. I switched over to the branch cgolubi1/2398_site_upgrade and at least the errors are gone, although I seem to have a huge number of extra errors that you don't seem to have.

PHPUnit 5.7.27 by Sebastian Bergmann and contributors.

...................I....I...........II.........................  63 / 744 (  8%)
........F......F..F...F......F...F............................. 126 / 744 ( 16%)
............................................................... 189 / 744 ( 25%)
................F.F.F.F.F......F.F.F........................... 252 / 744 ( 33%)
............................................................... 315 / 744 ( 42%)
.................III........................................... 378 / 744 ( 50%)
............................................................... 441 / 744 ( 59%)
..........................................I.......I............ 504 / 744 ( 67%)
............................................................... 567 / 744 ( 76%)
............................................................... 630 / 744 ( 84%)
............................................................... 693 / 744 ( 93%)
...................................................             744 / 744 (100%)

Time: 1.55 minutes, Memory: 46.00MB

There were 14 failures:

1) responder02Test::test_interface_game_026
API call should succeed:
ARGS: array (
  'type' => 'createGame',
  'playerInfoArray' =>
  array (
    0 =>
    array (
      0 => 'responder003',
      1 => 'RandomBMMixed',
    ),
    1 =>
    array (
      0 => 'responder004',
      1 => 'RandomBMMixed',
    ),
  ),
  'maxWins' => 3,
  'description' => '',
)
RETURN: array (
  'data' => NULL,
  'message' => 'Game create failed: Called bm_rand() from a test requiring overrides, but BM_RAND_VALS is empty',
  'status' => 'failed',
)
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'ok'
+'failed'

/Applications/MAMP/htdocs/ButtonMen/test/src/api/responderTestFramework.php:916
/Applications/MAMP/htdocs/ButtonMen/test/src/api/responderTestFramework.php:948
/Applications/MAMP/htdocs/ButtonMen/test/src/api/responder02Test.php:755

2) responder02Test::test_interface_game_033
API call should succeed:
ARGS: array (
  'type' => 'createGame',
  'playerInfoArray' =>
  array (
    0 =>
    array (
      0 => 'responder003',
      1 => 'RandomBMMixed',
    ),
    1 =>
    array (
      0 => 'responder004',
      1 => 'RandomBMMixed',
    ),
  ),
  'maxWins' => 3,
  'description' => '',
)
RETURN: array (
  'data' => NULL,
  'message' => 'Game create failed: Invalid die value: 5 is not between 1 and 4 for die ^kv(4)',
  'status' => 'failed',
)
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'ok'
+'failed'

/Applications/MAMP/htdocs/ButtonMen/test/src/api/responderTestFramework.php:916
/Applications/MAMP/htdocs/ButtonMen/test/src/api/responderTestFramework.php:948
/Applications/MAMP/htdocs/ButtonMen/test/src/api/responder02Test.php:1901

3) responder02Test::test_interface_game_036
Failed asserting that 2 matches expected 0.

/Applications/MAMP/htdocs/ButtonMen/test/src/api/responderTestFramework.php:917
/Applications/MAMP/htdocs/ButtonMen/test/src/api/responderTestFramework.php:948
/Applications/MAMP/htdocs/ButtonMen/test/src/api/responder02Test.php:2975

4) responder02Test::test_interface_game_040
API call should succeed:
ARGS: array (
  'type' => 'createGame',
  'playerInfoArray' =>
  array (
    0 =>
    array (
      0 => 'responder003',
      1 => 'RandomBMMixed',
    ),
    1 =>
    array (
      0 => 'responder004',
      1 => 'RandomBMMixed',
    ),
  ),
  'maxWins' => 3,
  'description' => '',
)
RETURN: array (
  'data' => NULL,
  'message' => 'Game create failed: Invalid die value: 7 is not between 1 and 4 for die Gpv(4)',
  'status' => 'failed',
)
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'ok'
+'failed'

/Applications/MAMP/htdocs/ButtonMen/test/src/api/responderTestFramework.php:916
/Applications/MAMP/htdocs/ButtonMen/test/src/api/responderTestFramework.php:948
/Applications/MAMP/htdocs/ButtonMen/test/src/api/responder02Test.php:3572

5) responder03Test::test_interface_game_047
Failed asserting that two arrays are equal.
--- Expected
+++ Actual
@@ @@
         1 => 'Skill'
-        2 => 'Trip'
+        2 => 'Rush'
     )
     'gameSkillsInfo' => Array (
-        'Berserk' => Array (...)
-        'Konstant' => Array (...)
         'Maximum' => Array (
             'code' => 'M'
             'description' => 'Maximum dice always roll thei...value.'
             'interacts' => Array (
-                'Konstant' => 'Dice with both Konstant and M...rolled'
             )
         )
-        'Mighty' => Array (...)
         'RandomBMMixed' => Array (...)
-        'Trip' => Array (...)
         'Value' => Array (
             'code' => 'v'
             'description' => 'These dice are not scored lik...size).'
             'interacts' => Array (
+                'Poison' => 'Dice with both Poison and Val... sides'
             )
         )
+        'Focus' => Array (...)
+        'Poison' => Array (...)
+        'Queer' => Array (...)
+        'Rush' => Array (...)
@@ @@
             'waitingOnAction' => true
-            'roundScore' => 26
-            'sideScore' => 11.0
+            'roundScore' => 22.5
+            'sideScore' => 10.300000000000000710542735760100185871124267578125
@@ @@
                 'name' => 'RandomBMMixed'
-                'recipe' => 'Ht(4) Mt(8) M(10) H(10) (20)'
-                'originalRecipe' => 'Ht(4) Mt(8) M(10) H(10) (20)'
+                'recipe' => 'Mv(4) fv(8) f(10) M(10) (20)'
+                'originalRecipe' => 'Mv(4) fv(8) f(10) M(10) (20)'
                 'artFilename' => 'BMdefaultRound.png'
             )
             'activeDieArray' => Array (
                 0 => Array (
-                    'value' => 1
+                    'value' => 4
                     'sides' => 4
                     'skills' => Array (
-                        0 => 'Mighty'
-                        1 => 'Trip'
+                        0 => 'Maximum'
+                        1 => 'Value'
                     )
                     'properties' => Array (
+                        0 => 'ValueRelevantToScore'
                     )
-                    'recipe' => 'Ht(4)'
-                    'description' => 'Mighty Trip 4-sided die'
+                    'recipe' => 'Mv(4)'
+                    'description' => 'Maximum Value 4-sided die'
                 )
                 1 => Array (
-                    'value' => 8
+                    'value' => 1
                     'sides' => 8
                     'skills' => Array (
-                        0 => 'Maximum'
-                        1 => 'Trip'
+                        0 => 'Focus'
+                        1 => 'Value'
                     )
                     'properties' => Array (
+                        0 => 'ValueRelevantToScore'
                     )
-                    'recipe' => 'Mt(8)'
-                    'description' => 'Maximum Trip 8-sided die'
+                    'recipe' => 'fv(8)'
+                    'description' => 'Focus Value 8-sided die'
                 )
                 2 => Array (
-                    'value' => 10
+                    'value' => 1
                     'sides' => 10
                     'skills' => Array (
-                        0 => 'Maximum'
+                        0 => 'Focus'
                     )
                     'properties' => Array ()
-                    'recipe' => 'M(10)'
-                    'description' => 'Maximum 10-sided die'
+                    'recipe' => 'f(10)'
+                    'description' => 'Focus 10-sided die'
                 )
                 3 => Array (
-                    'value' => 1
+                    'value' => 10
                     'sides' => 10
                     'skills' => Array (
-                        0 => 'Mighty'
+                        0 => 'Maximum'
                     )
                     'properties' => Array ()
-                    'recipe' => 'H(10)'
-                    'description' => 'Mighty 10-sided die'
+                    'recipe' => 'M(10)'
+                    'description' => 'Maximum 10-sided die'
@@ @@
             'waitingOnAction' => false
-            'roundScore' => 9.5
-            'sideScore' => -11.0
+            'roundScore' => 7
+            'sideScore' => -10.300000000000000710542735760100185871124267578125
@@ @@
                 'name' => 'RandomBMMixed'
-                'recipe' => '(4) Bk(4) Bk(6) v(10) v(20)'
-                'originalRecipe' => '(4) Bk(4) Bk(6) v(10) v(20)'
+                'recipe' => '(4) #p(4) #p(6) q(10) q(20)'
+                'originalRecipe' => '(4) #p(4) #p(6) q(10) q(20)'
@@ @@
                     'skills' => Array (
-                        0 => 'Berserk'
-                        1 => 'Konstant'
+                        0 => 'Rush'
+                        1 => 'Poison'
                     )
                     'properties' => Array ()
-                    'recipe' => 'Bk(4)'
-                    'description' => 'Berserk Konstant 4-sided die'
+                    'recipe' => '#p(4)'
+                    'description' => 'Rush Poison 4-sided die'
                 )
                 2 => Array (
                     'value' => 5
                     'sides' => 6
                     'skills' => Array (
-                        0 => 'Berserk'
-                        1 => 'Konstant'
+                        0 => 'Rush'
+                        1 => 'Poison'
                     )
                     'properties' => Array ()
-                    'recipe' => 'Bk(6)'
-                    'description' => 'Berserk Konstant 6-sided die'
+                    'recipe' => '#p(6)'
+                    'description' => 'Rush Poison 6-sided die'
                 )
                 3 => Array (
                     'value' => 2
                     'sides' => 10
                     'skills' => Array (
-                        0 => 'Value'
+                        0 => 'Queer'
                     )
                     'properties' => Array (
-                        0 => 'ValueRelevantToScore'
                     )
-                    'recipe' => 'v(10)'
-                    'description' => 'Value 10-sided die'
+                    'recipe' => 'q(10)'
+                    'description' => 'Queer 10-sided die'
                 )
                 4 => Array (
                     'value' => 3
                     'sides' => 20
                     'skills' => Array (
-                        0 => 'Value'
+                        0 => 'Queer'
                     )
                     'properties' => Array (
-                        0 => 'ValueRelevantToScore'
                     )
-                    'recipe' => 'v(20)'
-                    'description' => 'Value 20-sided die'
+                    'recipe' => 'q(20)'
+                    'description' => 'Queer 20-sided die'
@@ @@
             'player' => ''
-            'message' => 'responder003 won initiative for round 1. Initial die values: responder003 rolled [Ht(4):1, Mt(8):8, M(10):10, H(10):1, (20):1], responder004 rolled [(4):3, Bk(4):2, Bk(6):5, v(10):2, v(20):3]. responder003 has dice which are not counted for initiative due to die skills: [Ht(4), Mt(8)].'
+            'message' => 'responder003 won initiative for round 1. Initial die values: responder003 rolled [Mv(4):4, fv(8):1, f(10):1, M(10):10, (20):1], responder004 rolled [(4):3, #p(4):2, #p(6):5, q(10):2, q(20):3].'
         )
         1 => Array (...)
     )
     'gameActionLogCount' => 2
     'gameChatLog' => Array ()
     'gameChatLogCount' => 0
     'gameChatEditable' => false
     'dieBackgroundType' => 'realistic'
 )

/Applications/MAMP/htdocs/ButtonMen/test/src/api/responderTestFramework.php:1076
/Applications/MAMP/htdocs/ButtonMen/test/src/api/responder03Test.php:2339

6) responder03Test::test_interface_game_050
API call should succeed:
ARGS: array (
  'type' => 'createGame',
  'playerInfoArray' =>
  array (
    0 =>
    array (
      0 => 'responder003',
      1 => 'RandomBMMixed',
    ),
    1 =>
    array (
      0 => 'responder004',
      1 => 'Craps',
    ),
  ),
  'maxWins' => 3,
  'description' => '',
)
RETURN: array (
  'data' => NULL,
  'message' => 'Game create failed: Called bm_rand() from a test requiring overrides, but BM_RAND_VALS is empty',
  'status' => 'failed',
)
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'ok'
+'failed'

/Applications/MAMP/htdocs/ButtonMen/test/src/api/responderTestFramework.php:916
/Applications/MAMP/htdocs/ButtonMen/test/src/api/responderTestFramework.php:948
/Applications/MAMP/htdocs/ButtonMen/test/src/api/responder03Test.php:2887

7) BMBtnSkillRandomBMDuoskillTest::testSpecify_recipes_valid_args
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'(6) Hc(10) (12) H(20) c(X)'
+'(6) kq(10) (12) q(20) k(X)'

/Applications/MAMP/htdocs/ButtonMen/test/src/engine/BMBtnSkillRandomBMDuoskillTest.php:51

8) BMBtnSkillRandomBMFixedTest::testSpecify_recipes_valid_args
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'f(6) (6) f(10) (12) (20)'
+'c(6) (6) c(10) (12) (20)'

/Applications/MAMP/htdocs/ButtonMen/test/src/engine/BMBtnSkillRandomBMFixedTest.php:49

9) BMBtnSkillRandomBMMixedTest::testSpecify_recipes_valid_args
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'k(6) kz(6) (10) cz(12) c(20)'
+'n(6) gn(6) (10) go(12) o(20)'

/Applications/MAMP/htdocs/ButtonMen/test/src/engine/BMBtnSkillRandomBMMixedTest.php:58

10) BMBtnSkillRandomBMMonoskillTest::testSpecify_recipes_valid_args
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'(6) (10) H(12) H(20) (X)'
+'(6) (10) f(12) f(20) (X)'

/Applications/MAMP/htdocs/ButtonMen/test/src/engine/BMBtnSkillRandomBMMonoskillTest.php:50

11) BMBtnSkillRandomBMPentaskillTest::testSpecify_recipes_valid_args
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'c(6) bk(10) H(12) BH(20) Bbck(X)'
+'k(6) #H(10) s(12) ds(20) #Hdk(X)'

/Applications/MAMP/htdocs/ButtonMen/test/src/engine/BMBtnSkillRandomBMPentaskillTest.php:54

12) BMBtnSkillRandomBMTest::testRandomly_select_skills
Failed asserting that two arrays are equal.
--- Expected
+++ Actual
@@ @@
 Array (
     0 => 's3'
-    1 => 's2'
+    1 => 's4'
 )

/Applications/MAMP/htdocs/ButtonMen/test/src/engine/BMBtnSkillRandomBMTest.php:79

13) BMBtnSkillRandomBMTetraskillTest::testSpecify_recipes_valid_args
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'c(6) k(10) H(12) BH(20) Bck(X)'
+'M(6) d(10) c(12) cf(20) Mdf(X)'

/Applications/MAMP/htdocs/ButtonMen/test/src/engine/BMBtnSkillRandomBMTetraskillTest.php:53

14) BMBtnSkillRandomBMTriskillTest::testSpecify_recipes_valid_args
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'c(6) k(10) H(12) H(20) ck(X)'
+'f(6) n(10) #(12) #(20) fn(X)'

/Applications/MAMP/htdocs/ButtonMen/test/src/engine/BMBtnSkillRandomBMTriskillTest.php:52

FAILURES!
Tests: 744, Assertions: 13838, Failures: 14, Incomplete: 9.
blackshadowshade commented 6 years ago

Looks like I'm having issues with the random number generation.

cgolubi1 commented 6 years ago

That's probably not the random number generator. You're specifically getting the wrong RandomBM types. I would guess something is wrong with your definition of the bm_skill_rand() function, which is defined for you in deploy/amp/phpunit_bootstrap.php --- my complete wild guess is that for some reason your bm_skill_rand() is trying to use $BM_RAND_VALS instead of $BM_SKILL_RAND_VALS. I don't know how that would have happened, but i think it would explain what you're seeing.

[Edit: actually, or if you weren't overriding bm_skill_rand() at all, like if that function got dropped from your phpunit_bootstrap.php, that might also explain it.]

blackshadowshade commented 6 years ago

Huh, interesting. I should be able to look at that tonight. It's quite possible I'm not calling the correct bootstrap file, considering all the changes in infrastructure that I've been going through to get everything up and running.

blackshadowshade commented 6 years ago

Good news (I think). Once I started using the bootstrap file again, all my tests pass.

PHPUnit 5.7.27 by Sebastian Bergmann and contributors.

...................I....I...........II.........................  63 / 744 (  8%)
............................................................... 126 / 744 ( 16%)
............................................................... 189 / 744 ( 25%)
............................................................... 252 / 744 ( 33%)
............................................................... 315 / 744 ( 42%)
.................III........................................... 378 / 744 ( 50%)
............................................................... 441 / 744 ( 59%)
..........................................I.......I............ 504 / 744 ( 67%)
............................................................... 567 / 744 ( 76%)
............................................................... 630 / 744 ( 84%)
............................................................... 693 / 744 ( 93%)
...................................................             744 / 744 (100%)

Time: 2.12 minutes, Memory: 46.00MB

OK, but incomplete, skipped, or risky tests!
Tests: 744, Assertions: 13968, Incomplete: 9.
blackshadowshade commented 6 years ago

So, I went back and ran the tests on our current master branch, and got errors and failures, as expected:

PHPUnit 5.7.27 by Sebastian Bergmann and contributors.

...................I....I...........II.........................  63 / 744 (  8%)
............................................................... 126 / 744 ( 16%)
............................................................... 189 / 744 ( 25%)
............................................................... 252 / 744 ( 33%)
............................................................... 315 / 744 ( 42%)
.................III........................................... 378 / 744 ( 50%)
............................................................... 441 / 744 ( 59%)
..........................................I.......I............ 504 / 744 ( 67%)
.............E....E.................F.........................F 567 / 744 ( 76%)
...........E.................F...........E............F........ 630 / 744 ( 84%)
....F.......................................................... 693 / 744 ( 93%)
...................................................             744 / 744 (100%)

Time: 1.72 minutes, Memory: 46.00MB

There were 4 errors:

1) BMGameTest::test__set_active_die_array_array
TypeError: Argument 2 passed to BMGame::setBMPlayerProps() must be of the type array, string given, called in /Applications/MAMP/htdocs/ButtonMen/src/engine/BMGame.php on line 3288

/Applications/MAMP/htdocs/ButtonMen/src/engine/BMGame.php:4537
/Applications/MAMP/htdocs/ButtonMen/src/engine/BMGame.php:3288
/Applications/MAMP/htdocs/ButtonMen/test/src/engine/BMGameTest.php:2552

2) BMGameTest::test__set_captured_die_array_array
TypeError: Argument 2 passed to BMGame::setBMPlayerProps() must be of the type array, string given, called in /Applications/MAMP/htdocs/ButtonMen/src/engine/BMGame.php on line 3288

/Applications/MAMP/htdocs/ButtonMen/src/engine/BMGame.php:4537
/Applications/MAMP/htdocs/ButtonMen/src/engine/BMGame.php:3288
/Applications/MAMP/htdocs/ButtonMen/test/src/engine/BMGameTest.php:2730

3) BMGameTest::test_default_round
Parameter 1 to end() expected to be a reference, value given

/Applications/MAMP/htdocs/ButtonMen/test/src/engine/BMGameTest.php:9986

4) BMInterfaceGameTest::test_react_to_auxiliary_invalid
TypeError: Argument 1 passed to BMInterfaceGame::is_action_current() must be an instance of BMGame, boolean given, called in /Applications/MAMP/htdocs/ButtonMen/src/engine/BMInterfaceGame.php on line 1043

/Applications/MAMP/htdocs/ButtonMen/src/engine/BMInterfaceGame.php:1581
/Applications/MAMP/htdocs/ButtonMen/src/engine/BMInterfaceGame.php:1043
/Applications/MAMP/htdocs/ButtonMen/test/src/engine/BMInterfaceGameTest.php:1056

--

There were 5 failures:

1) BMGameTest::test_berserk_round
Failed asserting that false is true.

/Applications/MAMP/htdocs/ButtonMen/test/src/engine/BMGameTest.php:5877

2) BMGameTest::test_option_game
Failed asserting that false is true.

/Applications/MAMP/htdocs/ButtonMen/test/src/engine/BMGameTest.php:9027

3) BMInterfaceGameTest::test_create_game_with_two_random_buttons
Failed asserting that true is false.

/Applications/MAMP/htdocs/ButtonMen/test/src/engine/BMInterfaceGameTest.php:437

4) BMInterfaceTest::test_swing_value_reset_at_end_of_round
Failed asserting that false is true.

/Applications/MAMP/htdocs/ButtonMen/test/src/engine/BMInterfaceTest.php:412

5) BMInterfaceTest::test_option_game
Failed asserting that false is true.

/Applications/MAMP/htdocs/ButtonMen/test/src/engine/BMInterfaceTest.php:1416

ERRORS!
Tests: 744, Assertions: 13760, Errors: 4, Failures: 5, Incomplete: 9.
blackshadowshade commented 6 years ago

@cgolubi1, it looks promising that I seem to be replicating the behaviour of PHPUnit that you're seeing.

If your replay engine passes, I'm happy for this site upgrade go ahead. We'll still want to ask testers to hit the site, of course.

Also, I'm not forgetting about the isset() and empty() audit, I still intend to go through and check all of those at some stage.