DM-25010: Make Alert serialization optional in diaPipe.

[morriscb]

No description provided.

[jdswinbank]

Is doSerializeAlerts the write term here?

I mean, it's correct, but it seems to add an unnecessary third term beyond “package” (on the basis that it's PackageAlertsTask, rather than SerializeAlertsTask that we're referring to) and “write” (on the basis that it's really writing out the alerts that the user cares about)

It's not a big deal, it just feels like we're using three terms which all kind-of-but-not-quite mean the same thing, where one (or maybe two) would do fine.

[morriscb]

Changed everything to PackageAlerts.

[jdswinbank]

Maybe be explicit that they are being written to the filesystem (for now, at least)?

[jdswinbank]

Would you consider using doSerializeAlerts here? (Or whatever you change that to, if you think my comment on naming is helpful). Two reasons: one for consistency, and the other because doAlerts is not very descriptive (“do alerts what?”).

[jdswinbank]

If I am reading this correctly, that assert_not_called() is meaningless — what's happening here is an AttributeError is raised as soon as an attempt is made to access subtasks.alertPackager, because that substask was never created. Is that right?

If so, could you please change this logic to make this logic more straightforward? I'd suggest that avoiding the exception altogether — something like

if doAlerts:
    ...
else:
    self.assertFalse(hasattr(subtasks, 'alertPackager'))

— but if you prefer the exception you please take out the attribute accesses that will never fire and make clear in a comment what's happening:

def testRunWithoutAlerts(self):
    # When alert generation is disabled, `_testRun` is expected to raise
    # an AttributeError due to the lack of an `alertPackager` subtask.
    with self.assertRaises(AttributeError):
        self._testRun(False)

...

if doAlerts:
    ...
else:
    subtasks.alertPackager  # should raise AttributeError

[morriscb]

Picked the first option

[jdswinbank]

Maybe “with serializing/packaging/writing/whatever alerts” rather than “touching”? Just to make it a bit clear what's actually happening.

[jdswinbank]

As above, re “touching”.

[jdswinbank]

Hey Chris — thanks for these changes.

Looking at the changes you made to TestDiaPipelineTask, I realised that I hadn't previously understood what it was doing. Unfortunately, that means that the change I suggested (specifically, the self.assertFalse(hasattr(subtasks, 'alertPackager'))) is not very useful — it's only demonstrating that the test code works, not that the task itself is doing the right thing.

I spent a few minutes playing about with the code and came up with these changes: db2c0c2. I think they make the test a bit simpler and more useful than it is now. What do you think?

[morriscb]

Sure, that looks good to me. Since it's on your branch, do you mind appending it to the end of this ticket branch? You're much better with git after all :D

[jdswinbank]

Appended!