On Thu, 3 Jun 2021 at 09:21, Daniel Gruno <[email protected]> wrote: > > On 03/06/2021 00.21, sebb wrote: > > On Wed, 2 Jun 2021 at 18:32, Daniel Gruno <[email protected]> wrote: > >> > >> I've made it so that the tests will pass with 56 specific format=flowed > >> issues in mind. > > > > Wrong, so wrong. > > Depends on what the goal of the test is. We seem to disagree on what > they are there for, and that's fair. I'm not hung up on pony and foal > being exactly equal in their results,
Huh? That's precisely the issue here. > I use the tests to guard against > unintended changes. When the change is very much intended, I expect the > tests to allow for that. I agree that if a change is expected, it should be allowed for. That is what was done for v0.10, as there is no way to change the past. But it does not apply here. A re-implementation of an existing generator MUST produce the same output for the same input. Otherwise it is broken. It's unfortunate that there were no unit tests until recently, otherwise the changes to the output would have been noticed before they could cause any issues. > > > >> We have the option of: > >> - Be strict and let foal fail forever > >> - Be flexible and have foal and pony differ in these specific cases > >> - Create a separate set of tests for both > > > > Or, fix foal so it generates the same results for the same generators. > > Fixing here would be "fixing" by breaking improvements - I am not > willing to do that. I'd rather we yank out the legacy, medium and > cluster generators and then get sean's DKIM patches merged in and settle > on DKIM and full as the new standard generators. A generator is not 'improved' by changing the code in such a way as to alter the output. It is either a bug, or a different generator. The output of the generator for a given input is a fundamental part of its public API. SHA1 is an improvement over MD5, but you cannot 'improve' an MD5 generator by producing a SHA1 hash instead. Note that the FULL generator must continue to generate the same output as previously. The DKIM generator is not thus encumbered currently, as it has not been released (I hope). > I'll look at those patches as time allows today. > > This won't solve the parsing tests however, where the two versions > produce different results due to how they parse format=flowed and how > some character encodings are handled. Here we either need to keep the > alternate flag, or split it into tests for pony and tests for foal, and > have runall figure that out somehow. I agree that the parsing tests are different. Most output should be unaffected, but there will be cases where output can be improved. In such cases, it would be better to have a version-specific attribute. Otherwise it could hide changes where they are not expected. For example, a change to Foal that accidentally reverts a fix so the old output is generated. That would be a bug, but it would not be detected. > > > > I realise that foal has deprecated the old generators, however it > > still offers them as options. > > > > There is no point in doing so if the results are different, which is > > why the tests were set up. > > Fixing the tests does not make the code correct, it just hides the problems. > > > > We have to make allowances for v 0.10, because incompatible changes > > were made previously. > > It's not possible to change the past, so we have to live with it. > > > > However, for new code, it is possible to get it right -- or at least > > not make it more wrong. > > > >> I am leaning towards option 2, which is how it is right now. Option 1 > >> means we can't use the tests for foal, and option 3 would mean we risk > >> not noticing when the two diverge. > >> > >> For the places where the tests diverge (and need alternates), I've put a > >> top comment in each YAML specifying who/why. I think this is currently > >> the best compromise. > > > > There is no need to compromise. > > > > Foal needs to generate the same output for the same input for all the > > historic generators that it supports. > > > > Anything else just makes an already bad situation worse. > > > >> >
