[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: Apropos 54f297904e0c: Temporarily comment out CC Mode from tests whi

From: João Távora
Subject: Re: Apropos 54f297904e0c: Temporarily comment out CC Mode from tests which are incompatible with it.
Date: Thu, 17 Jan 2019 17:57:24 +0000

Hello again, Alan

On Thu, Jan 17, 2019 at 4:54 PM Alan Mackenzie <address@hidden> wrote:

> >     Temporarily comment out CC Mode from tests which are incompatible
> > with it.
> That would leave lots of failed tests in make check.  People have
> already remarked on those failures, implicitly asking me to fix them.

Yes. But the correct fix was to revert the commit that broke
the tests, not this volkswagenesque atrocity.

> My understanding, from a previous encounter, was that having no failing
> unit tests was of a high priority.  I've only commented a little bit
> out, I haven't made any permanent, unreverseable changes.

Then you'll certainly be OK if I revert your two commits myself:
the commit that broke the tests and the commit that disables the test.
That is certainly not permanent or unreversable, too!

> With that test in, I got the error message: "No test named
> `electric-pair-whitespace-chomping-2-at-point-4-in-c++-mode-in-strings'",
> and no other tests were performed, leaving an electric-tests.log file 86
> bytes long.  That's why I commented it out.  This may be some glitch in
> the testing system.

I see, you disabled all c++ tests, *sigh*

> > C Mode doesn't use electric-layout mode, but a user can surely
> > decide he wants to use it in c-mode, can he not??
> No.  Certainly not at the moment.

No? Will you come to his house and tell him personally not to
M-x electric-layout-mode? What if that's what he _prefers_?

> > These tests pass fine currently.
> When I ran them, there were lots of failures, because the tests assumed
> electric-layout-mode was active.

The tests should activate electric-layout-mode temporarily if needed and
the revert to the previous situation.  If they aren't doing this, that is what
needs to be fixed, but it works for me and I don't have electric-layout-mode,

> > Please revert this fix and lets discuss why you need to disable tests.
> It's not a fix, it's a temporary workaround.

With all due respect, I've heard that one before many many times.

Do you realize that you've broken many many things about

For example Beatrix, and me and everyone else will now lose
the default "autobalancing" functionality of electric-pair-mode,
which is its best feature! Autobalance used to work identically in
most modes, but now you broke that. Is it worth breaking so
many things just for fixing a much smaller case?

Just one example of one of the many cases you broke.

electric-pair-mixed-paren-3-at-point-4-in-c++-mode is a test defined
in `electric-tests.elc'.

Electricity test in a `c++-mode' buffer.

Start with point at 4 in a 7-char-long buffer
like this one:

  |  (])  |   (buffer start and end are denoted by `|')

Now call this:

#[nil "\300\301!\207"
      [electric-pair-mode 1]

Now press the key for: (

The buffer's contents should become:

  |  (()])  |

, and point should be at 5.


You turn this into (())]) so now I have two unbalanced parenthesis types
in my buffer.

> Anyhow, surely the implementation of Emacs should not be constrained by
> its tests?  Rather the tests should test the chosen implementation.

Really Alan, you completely broke electric-pair-mode by trying to
reimplement it in cc-mode.el where it was working perfectly but for a small
glitch (which really isn't its fault).

If you're going to reimplement it, reimplement *all* of it (of course you might
come to the conclusion that its best to use the existing implementation...)

> > If we come to the conclusion that some tests are asserting unreasonable
> > expectations about the functionality you develop, we can disable them on a
> > case by case basis!
> I would have done that, indeed tried to do that, but the lack of
> documentation of electric-pair-test-for, electric-pair-define-test-form
> and so on, many of them with 8, 9 or more parameters, made that too
> difficult, given the urgency I felt to produce a workaround.

If this is indeed urgent, you can add some conditional check to the
offending code using 'ert-running-test'.

> > If on the other hand, if you need to do some work "temporarily", then
> > the best way to do it without disturbing other people's developments
> > is to do it in a separate branch.
> I fixed bug #33794[*] on master, as I always do with CC Mode bugs (and most
> other sorts of bugs, too).  That fix is, in principle, sound.  I didn't
> discover the problems with the unit tests till later (though I admit I
> should have done).

It's *not* sound if it breaks tests. At least not without first arguing
that the tests are placing unreasonable expectations on your code.

> [*] That is, Beatrix Klebe's bug about CC Mode's auto-newlines not
> working with electric-pair-mode.

Beatrix Klebe, can, as a workaround, use electric-layout-mode
perfectly well for her use case (even though you insist she can't)
Until you can sort out c++-mode.

It's much easier than creating this mess that really interferes
with other's peoples work.

> Anyhow, apologies, and all that, but I don't want to spend any more time
> on this topic today or until tomorrow evening, since I've got an exam
> coming up tomorrow.  But I promise I'll get back to you (including
> answering your other post) either late tomorrow or on Saturday.

Alright. Good luck for your exam.

In the meantime. I will have to fix this differently. I will add a
temporary variable that I can set to have sane C++ behaviour
in the meantime. I will set this variable when running tests, so
the test people will see correct results.


reply via email to

[Prev in Thread] Current Thread [Next in Thread]