Diversion

GeePaw Hill has proposed a little problem, and he’d like to see how some of us solve it. Here we go! Outcome: I get a nice solution, much as I imagined, but my process was quite poor. Weird.

I want you to know that I got up early to work on this. I thought about it a bit last night and started thinking again after I woke up for other purposes. I don’t want to over-think it, because I want to share my early design thoughts, not some over-processed thoughts like some other guy might put in The Book of Perfect Programming.

Here’s GeePaw’s spec:

The CollatedTests class is a class that is used to collect test results across multiple Junit runs. During each Junit run, log records are collected that indicate which tests passed, failed, were disabled, or were aborted. (I do not know what aborted means in this context, and have never seen an aborted test.)

Imagine a TestStatus, which is Pass, Fail, Disable, Abort, and one other, Unrun.

The CollatedTests class has two functions. 1) add(testname:String, status:TestStatus), and 2) endRun():List<TestResult>. This is because the data comes in as four arrays associated with the first four TestStatus enums above. (No Unruns are added.)

endRun() is the DS&A part. It must return a list of TestResult, which has three fields: Name of the test, TestStatus, and a boolean isNew. Further, that list must be in the same order every time it’s called. That is, if the 0’th test was “MyTest.testSomething()”, that must be the 0th entry at every call to endRun(). Even if it was not run during that JUnit run, and hence is missing in log record.

Finally, if a test has never been run before a particular JUnit run, that TestResult must be marked as “isNew”.

Last night, as we all met on Zoom, I drew two little pictures on a card. Here they are:

Not much use, just whatever came to mind.

My thoughts last night went to Python dictionaries, which happen to keep keys in the order presented. After a while I realized that lists also happen to keep things in the order presented, so I mostly set aside whatever thinking I was doing with dictionaries.

GeePaw’s spec includes a class name, CollatedTests, but I will reject that name and substitute my own. Why? Because I think he is describing a machine, not a static bunch of tests. It is a machine with memory, because it remembers the order of test status items and always presents its results in what amounts to the order of arrival of names.

He specifies that the addition of TestStatus objects will be batched, and I plan not to care about that. We don’t preserve the batches and they seem to me to offer no value to us. (I could be wrong, but I’m probably not, in this case.)

So I envision a little machine where we add TestStatus objects, and it remembers the order of names coming in, and it notices when a name is new, because when it produces the results it is supposed to make new results with isNew = True. When we say endRun it’s supposed to return a report. endRun is a terrible name for getting a report, so we’ll probably come up with a new one.

My design thinking is somewhat visual. I kind of picture things in my head and sometimes I draw the pictures to clarify my thinking. Here’s a scribble I just made, or a fresh version depending on what I do when I put the pictures into the article:

So I now envision a little machine, with a list of tests from a run, and two other structures, old and new, containing all the “old” names and “new” names, respectively. I envision editing old and new as tests arrive, after tucking away the test specifics in the TS box. And when we’re asked for our report, I envision using first the old names to pull out the results, and then processing the new names, fetching results and moving the new names down to the old names. The new name process will set the isNew flag.

And there’s one more glitch … if the name in the old list is not found in the current batch, we’ll create a result for it, with the evil Unrun status.

An Issue

I know at least three ways to do this problem:

1. Kind of just type it in. It looks like it’s just a couple of lists and a dictionary to me, with some rigmarole attached. I will of course go in as small steps as I can think of.

2. TDD it in, using native collections like lists and dictionaries, in as small steps as I can think of.

3. TDD it in, using abstract collections—collections custom made for my purpose here—in as small steps as I can think of.

The first way is right out. If I do it that way, it will not be well tested. I’m sure I can make it work. If it was a one-time script, I might do it and be done but this is code that we’ll have in the system for a long time. It deserves a better existence than just being hammered out.

For our interest today, I am thinking about doing it both of the last two ways. The question is, which way would I do first? Whichever I do first will essentially solve the problem in my mind, so that I can’t really do it again with a clean slate.

I think that, even for publication, which most of my programming is, I would likely solve this problem using a small object with native collections inside. I do not think I would wrap those collections. I really do believe that things go better when we wrap collections right out of the box … and yet, quite often, I do not do it.

Why not? Because I think I “see” the answer, and I am hot to get there. Along the way, I often wind up with decent code, maybe a bit better than mediocre, but not code I can be proud of. I notice that recently, in particular. So, today … let’s start with abstract.

Begin With a Test

I try to say “begin with a test” to myself whenever I start something. Why? Because I probably have the solution in mind already, and I am hot to go, and if I do write the code first, I will write poor tests or no tests at all. And some fraction of the time, I put defects in the code and my weak tests do not find them. That is embarrassing.

I know that I do better with tests first. I know that I do better with very small steps. Sometimes I work with weak tests; sometimes I take big steps. Why do I do this, knowing better? Because I’m human. But I try to remember to do what works better and today, I plan to remember.

class TestCollated:
    def test_hookup(self):
        assert False

I get a red bar. Perfect. Now I need to invent a class name and I think the name is Collator, not CollatedTests. It’s a doer, not a holder.

    def test_collator_exists(self):
        Collator()

I have my tests on autorun, so of course that fails. I remove the hookup and make a tiny class.

Green. Now a fanatic would write a new test and since I’ve thought of it, I will do so as well. On another day I might rename that test and give it some action. I do have the habit of appending to tests, and I think we’re going to see that happening today, since our Collator has memory.

But this time, a new test.

    def test_no_tests(self):
        c = Collator()
        assert c.report() == []

I’ve already decided that endRun() is a poor name and I’m calling the method that produces results report. If there is really some reason to have a method endRun, we’ll implement it to call report or something.

So this isn’t too interesting yet. We need to add at least one test status, and one is a good place to start. Yes, we could write a more comprehensive test and work up to the answer, but this will work better, I think, because it almost always does.

I wrote down more than I can really eat in one bite:

    def test_one_status(self):
        c = Collator()
        c.add(TestStatus('T1', 'Pass'))
        report = c.report()
        result = report[0]
        assert result.name == 'T1'
        assert result.status == 'Pass'
        assert result.isNew

But the thing is, this does express the right result and while I could have skipped the isNew, it seemed better to express it. I could be wrong. I plan to implement using a bit of “Fake It Till You Make It”. Let’s see what I do.

First I need a TestStatus object.

class TestStatus:
    def __init__(self, name, status, is_new=False):
        self.name = name
        self.status = status
        self.is_new = is_new

PyCharm objected rightly to the name isNew, so I renamed to is_new. I changed the test a bit to match:

    def test_one_status(self):
        c = Collator()
        c.add(TestStatus('T1', 'Pass'))
        report = c.report()
        result = report[0]
        assert result.name == 'T1'
        assert result.status == 'Pass'
        assert result.is_new is True

Now we need add. Here there is a decision to e made. I have enough design in mind to think that I will be using a dictionary to hold the TestStatus instances, so that I can fetch them by name. But no: we’re doing abstractions here, aren’t we? Maybe not yet. Let’s just have a list, for now.

class Collator:
    def __init__(self):
        self.statuses = []

    def add(self, status):
        self.statuses.append(status)

Now we need a report method. We’ll fake it:

class Collator:
    def __init__(self):
        self.statuses = []

    def add(self, status):
        self.statuses.append(status)

    def report(self):
        return self.statuses

Test fails. I regret checking the is_new: I assume that’s what’s wrong.

>       assert result.is_new is True
E       assert False is True

Yes. Let’s remove that requirement from the test and deal with it later. We’ll not forget because we wrote it on a card, and anyway we’ll have more comprehensive tests as we go. The newly shorter test passes:

    def test_one_status(self):
        c = Collator()
        c.add(TestStatus('T1', 'Pass'))
        report = c.report()
        result = report[0]
        assert result.name == 'T1'
        assert result.status == 'Pass'

This may be a lesson wishing to be learned. I was greedy, adding the is_new check right off the bat. But my simple solution doesn’t really want to deal with that yet, so I remove the requirement from the current test rather than try to bash it in. It would be easy to bash in, but it seems likely that I’d have to bash it out pretty soon.

Reflection

We have progress, let’s commit: couple of tests working. Ewww, I accidentally committed a whole raft of stuff to the repo. I may regret that later, if we push this to GitHub. Locally, no harm done.

I think I’d like to test the order across two runs. I’m not really concerned about the order in one run: I don’t think I can get it wrong. So we need a two-phase test, I think.

    def test_two_phases(self):
        c = Collator()
        c.add(TestStatus('T1', 'Pass'))
        c.report()
        c.add(TestStatus('T2', 'Fail'))
        result = c.report()
        assert result[0].name == 'T1'
        assert result[1].name == 'T2'

I am a bit surprised when this test passes. Fine, we’ll keep it and do another. This one tells part of the story and the next one will break things so that we can move on.

    def test_two_harder_phases(self):
        c = Collator()
        c.add(TestStatus('T1', 'Pass'))
        c.report()
        c.add(TestStatus('T2', 'Pass'))
        c.add(TestStatus('T1', 'Fail'))
        result = c.report()
        assert len(result) == 2
        assert result[0].name == 'T1'
        assert result[0].status == 'Fail'
        assert result[1].name == 'T2'
        assert result[1].status == 'Pass'

This is failing on the length: we’re getting three.

And I just realized that we’re not yet returning a TestResult, we’re just returning the existing TestStatus. I think my adding the is_new flag there was a mistake. I’m going to ignore that last test and do the result. Better to get the basics in place as soon as we can.

I enhance this test to force out the new class:

    def test_one_status(self):
        c = Collator()
        c.add(TestStatus('T1', 'Pass'))
        report = c.report()
        result = report[0]
        assert type(result) == TestResult  # <===
        assert result.name == 'T1'
        assert result.status == 'Pass'

Now of course I could just do this and all the tests would just deal. But I’d rather take the moment to ensure that I’m getting what I want, and to express the fact that we return TestResults.

I modify TestStatus to remove the is_new and I create TestResult:

class TestStatus:
    def __init__(self, name, status):
        self.name = name
        self.status = status


class TestResult:
    def __init__(self, test_status):
        self.name = test_status.name
        self.status = test_status.status
        self.is_new = False

Now I plug it in:

    def report(self):
        return [TestResult(test_status) for test_status in self.statuses]

OK, we’re returning TestResults now. Fine. Stop nagging. Now unskip the newest test, which will still fail on length. We review the test to refresh our memory of what we were planning to do before we diverted to the TestResult issue.

    def test_two_harder_phases(self):
        c = Collator()
        c.add(TestStatus('T1', 'Pass'))
        c.report()
        c.add(TestStatus('T2', 'Pass'))
        c.add(TestStatus('T1', 'Fail'))
        result = c.report()
        assert len(result) == 2
        assert result[0].name == 'T1'
        assert result[0].status == 'Fail'
        assert result[1].name == 'T2'
        assert result[1].status == 'Pass'

Right, we need to do the old_names idea now. And it’s clear that we’ll need to empty the statuses collection after we do the report. Since I’ve thought of it, I’ll add it to the test. Calling report twice will return an empty report, since we have consumed the last group of statuses.

    def test_two_harder_phases(self):
        c = Collator()
        c.add(TestStatus('T1', 'Pass'))
        c.report()
        assert c.report == []  $ <===
        c.add(TestStatus('T2', 'Pass'))
        c.add(TestStatus('T1', 'Fail'))
        result = c.report()
        assert len(result) == 2
        assert result[0].name == 'T1'
        assert result[0].status == 'Fail'
        assert result[1].name == 'T2'
        assert result[1].status == 'Pass'

A better person than I am might do a smaller test here. In fact, to show you what a better person might do, they might do this:

    def test_report_clears_after_running(self):
        c = Collator()
        c.add(TestStatus('T1', 'Pass'))
        c.report()
        assert c.report == []

Have I forgotten to commit? Probably. Anyway we have red now so we can’t. I’m confident, so rather than ignore a couple of tests and commit, I’ll move forward. Might be better to do it the other way.

I think this is going to fix me up:

    def report(self):
        report = [TestResult(test_status) for test_status in self.statuses]
        self.statuses = []
        return report

But in fact that breaks three tests instead of fixing one!

It breaks test_two_phases, which is OK because that adds the tests in the wrong order and we’re not really handling order yet.

And the test_report_clears_after_running is wrong, should be:

    def test_report_clears_after_running(self):
        c = Collator()
        c.add(TestStatus('T1', 'Pass'))
        c.report()
        empty = c.report()
        assert empty == []

That is now green. And of course our final test is failing as well, the one checking two harder phases. Let’s just proceed: I think it will all sort out.

A wiser man might ignore a test, roll back, etc. I am sure that we’re really OK. If not, we’ll find out soon.

However, I do not have a nearby save point, since I failed to commit when I could have. So we’re in a bit of danger here. I think we’ll be fine.

We need to make the Collator start doing its actual job.

We want a collection of known names. I was calling it “old” above but “known” is probably better. What should “known” be? Well it has to retain order. For now, we’ll just make a list, but there are better ways. First we get a vine tossed across the gorge, later we make a bridge.

class Collator:
    def __init__(self):
        self.statuses = []
        self.known_names = []

    def add(self, status):
        self.statuses.append(status)
        if status.name not in self.known_names:
            self.known_names.append(status.name)

Necessary but not sufficient. We need to use the known names to fetch the statuses. Or we could make the results as we add rather than in the output. I think I like that better, but it seems like adding another change right now isn’t a great idea.

We want to drive the report from the known_names list. I’ll write the loop first and then maybe get back to a list comprehension.

I code report() by wishful thinking:

    def report(self):
        report = []
        for name in self.known_names:
            status = self.statuses[name]
            result = TestResult(status)
            report.append(result)
        self.statuses = []
        return report

The wishful part is self.statuses[name], because lists can’t do that. But dictionaries can.

class Collator:
    def __init__(self):
        self.statuses = dict()
        self.known_names = []

    def add(self, status):
        self.statuses[status.name] = status
        if status.name not in self.known_names:
            self.known_names.append(status.name)

Another test has broken. This is bad, very bad. I’m too far out on a weak limb of the tree here.

I forgot to re-init statuses to a dictionary:

    def report(self):
        report = []
        for name in self.known_names:
            status = self.statuses[name]
            result = TestResult(status)
            report.append(result)
        self.statuses = dict()
        return report

And now there is the case of an item not appearing:

    def test_two_phases(self):
        c = Collator()
        c.add(TestStatus('T1', 'Pass'))
        c.report()
        c.add(TestStatus('T2', 'Fail'))
        result = c.report()
        assert result[0].name == 'T1'
        assert result[1].name == 'T2'

This is actually expected, since we didn’t add a T1 the second time through. This is our opportunity to create a fake result. We’ll be Pythonic with try, then extract. First this:

    def report(self):
        report = []
        for name in self.known_names:
            try:
                status = self.statuses[name]
            except KeyError:
                status = TestStatus(name, "Unrun")
            result = TestResult(status)
            report.append(result)
        self.statuses = dict()
        return report

I hate that, because I don’t want to create a TestStatus, it’s not my job. Refactor to produce result instead.

    def report(self):
        report = []
        for name in self.known_names:
            try:
                result = TestResult(self.statuses[name])
            except KeyError:
                status = TestResult(name, "Unrun")
            result = TestResult(status)
            report.append(result)
        self.statuses = dict()
        return report

This is not going well. I really should have committed that save point, and should have gone in smaller steps. The current new issue is that my constructor for TestResult takes a TestStatus directly. I’m clinging by my fingernails, but I can do this:

class TestResult:
    def __init__(self, name, status, is_new=False):
        self.name = name
        self.status = status
        self.is_new = is_new

And:

    def report(self):
        report = []
        for name in self.known_names:
            try:
                status = self.statuses[name]
                result = TestResult(status.name, status.status)
            except KeyError:
                result = TestResult(name, "Unrun")
            report.append(result)
        self.statuses = dict()
        return report

I’m back down to two failures. What are they again?

    def test_report_clears_after_running(self):
        c = Collator()
        c.add(TestStatus('T1', 'Pass'))
        c.report()
        empty = c.report()
        assert empty == []

Ha! This test is no longer valid. We will always produce a full report, consisting of Unrun results. So let’s improve this test:

    def test_second_report_shows_unrun(self):
        c = Collator()
        c.add(TestStatus('T1', 'Pass'))
        c.report()
        unrun = c.report()
        assert len(unrun) == 1
        assert unrun[0].name == 'T1'
        assert unrun[0].status == 'Unrun'

And that passes. We’re back in control. That was scary. I’m going to skip the remaining test, commit, and reflect a bit.

    @pytest.mark.skip("waiting for after save point")
    def test_two_harder_phases(self):

Commit: Save point: reporting in known_names order.

Reflection

I got in more trouble than I wanted there. If I had committed once closer to when the trouble started, I might have felt better about rolling back. I think that GeePaw, who is much more organized than I am, would have rolled back, likely more than once. He does not like being out on a weak limb … nor do I, but I did what I did. My rules here are that I write up what I really do (as does GeePaw) and you get to see me making mistakes.

Just a bit more trouble, and I’d have had no choice but to roll back, or dig in for a very long debugging session. I know in my heart that rolling back and doing over is faster, so I hope I’d have done that. But I am aware that I might have just kept digging, and this session would be even longer than it already is.

I think we’ll move on now. There is an idea starting to form in my head: a NameKeeper. Right now we just have a list of known names. We know that somewhere near here, we will have a list of new names as well as known ones, and we could punch it in at this level. But I think we’ll do better with a little object.

NameKeeper will accept new names via, oh, add_name, and it will produce two lists upon request: known_names, and new_names. Let’s TDD that little object, because I want to get back to working in my best style, not my worst.

Continued in Diversion 2