| teward | cjwatson: this is the test output for the DMARC-changed code. There was early tests also which were also doctests erroring because the from addresses did actually change and didn't match - but when those were revised the remaining errors are the only ones I see failing AFAICT. https://people.ubuntu.com/~teward/lpdev-dmarc.txt | 00:08 |
|---|---|---|
| teward | (Chrome exploded when I tried to paste it into a pastebin) | 00:09 |
| teward | failed errors are towards the end | 00:09 |
| wgrant | teward: That looks fine | 00:10 |
| wgrant | - Subject: [Bug ...] subject | 00:10 |
| wgrant | ? ^^^ | 00:10 |
| wgrant | + Subject: [Bug 18] subject | 00:10 |
| wgrant | ? ^^ | 00:10 |
| mup | Bug #18: RFE: I'd like to be CC:d automatically to bugs I report <feature> <iso-testing> <lp-bugs> <Launchpad itself:Fix Released by bradb> <https://launchpad.net/bugs/18> | 00:10 |
| cjwatson | teward: Yep, as noted, this is doctest output being surprising to the uninitiated | 00:10 |
| wgrant | That's the full diff, not necessarily the diff that caused the failure. | 00:10 |
| wgrant | It doesn't include whitespace normalisation, and it doesn't include ellipsis. | 00:10 |
| teward | 👍 thanks cjwatson | 00:11 |
| teward | the only other failure cases are the From addresses | 00:11 |
| teward | which *are* what changed in my diff | 00:11 |
| cjwatson | You should find that if you fix just the From: bits then the rest will match | 00:11 |
| teward | that's the tricky part in *that* doctest | 00:11 |
| teward | unless it's always using 18 | 00:12 |
| cjwatson | <...@bugs.launchpad.net> | 00:12 |
| teward | ack | 00:12 |
| cjwatson | That's what ellipses are for | 00:12 |
| teward | those doctest errors are confuzling to me :) | 00:12 |
| wgrant | Sometimes I wonder if that was one of the design goals | 00:12 |
| wgrant | They can be remarkably obtuse. | 00:12 |
| teward | that i don't doubt xD | 00:16 |
| teward | but ODDLY no other bug notification tests errored heh | 00:16 |
| cjwatson | That is a little remarkable to me, but OK | 00:16 |
| cjwatson | I guess we don't *have* to have a bazillion tests that interact with everything. Usually there's more fallout from a change like this though | 00:18 |
| teward | cjwatson: indeed. yeah the lack of other things exploding here was also something that made me headscratch. Unless we don't *actually* test bug delivery? | 00:19 |
| cjwatson | Well, it evidently is being tested in that doctest | 00:20 |
| teward | yep | 00:20 |
| cjwatson | Unexpectedly light coverage, but we are actually testing it | 00:20 |
| cjwatson | Albeit slightly in passing | 00:21 |
| cjwatson | Though, I don't understand how your changes didn't cause bugnotification-email.txt to fail | 00:23 |
| teward | it did | 00:23 |
| teward | i fixed those | 00:23 |
| cjwatson | Oh, I see | 00:23 |
| teward | those were the straightforward fixes | 00:23 |
| cjwatson | OK, good | 00:23 |
| cjwatson | That's where get_bugmail_from_address is unit-tested (albeit in a ridiculous format, but they're conceptually unit tests) | 00:24 |
| teward | yeah and those (as expected) failed | 00:24 |
| teward | my git tree has two documentation test fixes - one for the latest errors (Ellipses are evil) and one with the earlier two from address mismatches (which were very easy fixes by replacing the email addresses) | 00:25 |
| cjwatson | And hopefully the surrounding prose commentary | 00:25 |
| teward | wanted to get the tests passing before I rewrite the commentary :) | 00:25 |
| teward | literally just filed a short time ago - https://code.launchpad.net/~teward/launchpad/+git/launchpad/+merge/383643 - but this should handle the DMARC compliant bug mail issue. The documentation needs vetted because I basically rewrote a whole portion of it. Kept all four test cases for From addresses, but the function is much less verbose and quite straightforward - I kept the test cases to show behavior. | 14:13 |
| teward | tests pass here, which was... surprising heh. | 14:14 |
| teward | if this goes into deployment however it would help having a nice document posted somewhere about the change as people might start headscratching when things change majorly. | 14:18 |
| teward | but that's a given :( | 14:18 |
| teward | :) * | 14:18 |
| cjwatson | Will look at it next week, thanks (public holiday here today) | 14:24 |
| teward | yep just putting it on the radar :) | 14:28 |
| teward | (wish I had a public holiday today... I don't want to be doing Work from Home today - could use an extra 4 hours sleep xD) | 14:28 |
| teward | cjwatson: do you know where Merge Proposal emails are constructed in the codebase? | 18:45 |
| teward | i see some in docs but I need to find the constructor (they're also DMARC-noncompliant) | 18:45 |
| cjwatson | teward: Somewhere in lib/lp/code/mail/ | 18:48 |
| teward | thanks i'll dig. that's going to be the next thing I start poking at. What's the testsuite for just merge proposals? (or merges) - lp.code ? | 18:51 |
| teward | (instead of lp.bugs like before) | 18:51 |
| cjwatson | Yes. The -t option takes a regex match applied to test case IDs - you can be more specific if you like, and I'd probably be inclined to do something like -t merge.\*proposal -t code.\*review | 18:53 |
| cjwatson | Should be rather faster | 18:53 |
| teward | +1 | 18:54 |
| teward | i'll dig there next, but i'm thinking a more phased approach to DMARC implementation - make sure there's no regressions in one part before implementing the rest. Unless we want to one-and-done it all but that has more regression risks. | 18:55 |
| cjwatson | We try to name tests at least somewhat regularly so that things like that have a better chance of matching | 18:55 |
| cjwatson | Doing this in pieces is absolutely correct and appropriate | 18:55 |
| cjwatson | No need to rush it | 18:55 |
| teward | yep | 18:58 |
| teward | i'm bored again though right now so :P Need something to pass time :P | 18:58 |
Generated by irclog2html.py 2.7 by Marius Gedminas - find it at mg.pov.lt!