Jeff leaving blocks most PRs
Opened this issue · 14 comments
Any PR that has not already passed (by virtue of being an add player PR) will fail once it hits the unit test rule because Jeff not being a player breaks the last test case for 0.3-allow-points-transfer. This failure occurs in validate-on-master, so a direct fix to the test case does not pass (see #266). I assume the same problem would occur from trying to remove the unit test rule entirely. So we need to re-add @jeffkaufman to the game until this rule is fixed, which is easiest to do if he does it himself, since that falls under the purview of adding a new player.
As part of #266 I've created some new test cases, but we probably need a more robust way to handle this generally than just assuming that I'm any more permanent of a player than Jeff is (I'm also not the only player referenced by the new test PRs).
Oh, I forgot about allow unanimous. That would also allow us to fix it. @dnathe4th @spam-man. Still, I think it's worth considering how to avoid this sort of thing in the future.
That's why it's important that https://github.com/jeffkaufman/nomic/blob/master/rules/0.1-allow-unanimous.py always be the first rule to run!
One way to fix this would be to have the test framework overwrite pieces of the derived PR to make sure we're testing exactly what we want. The nice thing is that the test is now more robust (dchudz would be allowed to return) but you do lose that the tests are currently fully end-to-end.
That could make sense. I was wondering whether we could create a dummy repo that mimicked the functionality but wasn't subject to the state of the real repo.
Thinking about it more, my idea would be pretty hard to implement. You'd want independent code per test case, or maybe per rule. I was thinking about trying to do everything against a single repo fork, but that would require maintaining the fork almost, but not quite, in parallel with the real repo (you wouldn't want a lot of divergence, but you would need at least a little as this issue demonstrates).
Though maybe that would be fine once we got everything to a stable position - every change would just require giving some thought to the fork beyond just merging down.
Yeah this is unfortunate as PRs are written point-in-time but the evaluation of the rule is given the current repo. My instinct is to allow for a test to run against a separate tests/players/ directory so the proper scenario could be orchestrated to make sure the test actually covers the case it intends. This might still be incomplete in more mechanics beyond just players and point transfers are added, but we can tackle that at that time.
I haven't looked at what it would take to parameterize the game state rules run on like this but I can take a look tonight if we think that's a good approach
The problem we're going to run into with players specifically is that some of our test cases care about PR authors. Maybe there's a way to fake or modify the author to get what we want conveniently?
Yeah, it probably wouldn't be hard to just change the author field in the pull_request object. Though I'd have concerns that doing so might open some backdoors into shenanigans.
On a related note, right now I'm just trusting that people won't abuse the test PRs by merging them (partially because most of them are inconsequential anyway), but it would be nice if we had a way to screen them out. A fork with its own set of PRs would do so, and to a lesser extent so would PRs against irrelevant code (like a dummy player set).
To keep people from merging your test PRs you can close them and then delete the branch.
That doesn't remove the PR data? How does that work?
It doesn't remove the PR data, no