Closed drewish closed 6 years ago
Thanks for separating this out. I'll give it a bit of thought.
Cool I'll throw a PR up so you can see what it looks like. It's pretty awesome for a free product.
The tests need a bit of looking at.
Some of them are rather too precise in what they expect -- for example, there's a complete transcript of solving Scott Adams's Adventureland, which is expected to remain the same. Whenever I change anything in the output format, those tests break and need to be tediously reconfigured. (Check through the git logs, and you'll see a lot of "Rebuild regression expectations" commits.)
I think what we need is a top-level rake target that rebuilds all of the test expectations automatically, so that once we are convinced the code is code we can make an output-format change, then run rake regenerate
, and know the tests will work.
Yeah the tests seemed pretty high level but I was happy there was at least something there ;)
For some reason I'd though the strings you were matching were trying to be exact matches with the original games. I didn't realize you'd customized the interface.
I did notice the Makefile for updating some of it. It shouldn't be too hard to convert that to a rake task if you're interested.
I see we've wandered from the nominal subject of this issue -- entirely my fault! I've filed #14 to discuss regenerating tests.
BTW, I've not deliberately customised the interface: in fact I've kept the strings as close as I could to the ones ScottFree uses, which I believe would have been copied directly from the TRS-80 versions of the games. But since there was no pre-existing canonical implementation that presented the output in scrolling-dialogue form (as opposed to the standard Adam split screen) it's inevitable that it differs in some respects.
I think this can be closed now that I've merged https://github.com/MikeTaylor/scottkit/pull/13 -- do you agree?
Yeah I think this is sufficient. Only follow up might be to change the github config to not allow merged with out passing tests. To prevent situations like my other PR ;)
Do you know how to make that change? (I know little to nothing about Travis.)
Sure thing, hit up https://github.com/MikeTaylor/scottkit/settings/branches/master and check the following boxes:
You can decide if you want to require it for your own PRs (using the "Include administrators" option), I usually do on my own projects just to avoid mistakes.
Weird -- when I do this, it says it can't find an any status-checks to enforce. Any ideas?
Oh right, yeah sorry forgot you hadn't enabled travis yet. Easiest way is to hit up https://travis-ci.org and use the sign in with GitHub button at the top. Once you grant access it should give you a list of your repos and switches to enable it. Flip the switch for the scottkit repo and it'll add web hooks to the GitHub repo and start running the tests and reporting status on the PRs.
Thanks, appreciated! It's nice that this process has had the side-effect of introducing me to Travis :-)
Well, I've seen tests running on a commit now! https://travis-ci.org/MikeTaylor/scottkit/builds/291618414
But of course they're all failing :-(
I'll wait for you to fix the output order to conform to what's expected.
Great, I'll get you a PR for that this evening
Beautiful!
For the historical record: https://github.com/MikeTaylor/scottkit/pull/15
I mentioned it in https://github.com/MikeTaylor/scottkit/issues/8#issuecomment-338507797 and know you like separate issues so decided to just open one up.