Re: master 262d0c6: Mark some tests as expensive

Re: master 262d0c6: Mark some tests as expensive

[Michael Albinus]

stefankangas@gmail.com (Stefan Kangas) writes:

Hi Stefan,

>     Mark some tests as expensive
>
>     * test/lisp/autorevert-tests.el
>     (auto-revert-test00-auto-revert-mode)
>     (auto-revert-test03-auto-revert-tail-mode)
>     (auto-revert-test04-auto-revert-mode-dired):
>     * test/lisp/cedet/semantic-utest-c.el
>     (semantic-test-c-preprocessor-simulation):
>     * test/lisp/cedet/srecode-utest-getset.el
>     (srecode-utest-getset-output):
>     * test/lisp/emacs-lisp/cl-seq-tests.el (cl-seq-test-bug24264):
>     * test/lisp/emacs-lisp/package-tests.el
>     (package-test-update-archives-async):
>     * test/lisp/filenotify-tests.el (file-notify-test03-events)
>     (file-notify-test04-autorevert)
>     (file-notify-test05-file-validity, file-notify-test08-backup):
>     * test/lisp/net/gnutls-tests.el (test-gnutls-005-aead-ciphers):
>     * test/lisp/shadowfile-tests.el (shadow-test00-clusters)
>     (shadow-test09-shadow-copy-files):

Could you please give a reasoning? Declaring tests as expansive decreases
heavily their application, giving us less chances to detect errors.

Best regards, Michael.

Re: master 262d0c6: Mark some tests as expensive

[Stefan Kangas]

Hi Michael,

Michael Albinus <michael.albinus@gmx.de> writes:

>>     Mark some tests as expensive
>
> Could you please give a reasoning? Declaring tests as expansive decreases
> heavily their application, giving us less chances to detect errors.

These tests all took 5-60 seconds to run, most in the lower end
admittedly.

The idea is to avoid that it will take a very long time to run the unit
tests as the test suite grows.  Arguably a unit test should never take
longer than a second, and even that is in the slow end.  If we have
10.000 unit tests taking a second each, just do the math of how long it
will take to run (even with parallelization).  We should not postpone
working on this until we are in that situation, IMHO, because by then it
will be a major pain.

I think the solution for important tests is to refactor the code to make
them run faster, for example by avoiding I/O or to make timers trigger
immediately.

Best regards,
Stefan Kangas

Re: master 262d0c6: Mark some tests as expensive

[Daniel Martín]

Stefan Kangas <stefan@marxist.se> writes:
>
> The idea is to avoid that it will take a very long time to run the unit
> tests as the test suite grows.  Arguably a unit test should never take
> longer than a second, and even that is in the slow end.  If we have
> 10.000 unit tests taking a second each, just do the math of how long it
> will take to run (even with parallelization).  We should not postpone
> working on this until we are in that situation, IMHO, because by then it
> will be a major pain.

I agree. We should differentiate between unit tests (they run in under a
second) and end-to-end tests, which are slower but still valuable
because they cover whole scenarios that unit tests can't cover. To keep
things scalable, we should strive to have plenty of fast unit tests and
fewer end-to-end tests. See
https://martinfowler.com/bliki/TestPyramid.html

>
> I think the solution for important tests is to refactor the code to make
> them run faster, for example by avoiding I/O or to make timers trigger
> immediately.

Yes, I think this is a good approach to follow in general. Many of the
test flakiness in the Emacs suite happen because many tests depend
implicitly in things outside the module they are testing (like
configuration, OS, etc.). Having more stable tests would help us detect
real regressions more accurately.

>
> Best regards,
> Stefan Kangas

Re: master 262d0c6: Mark some tests as expensive

[Eli Zaretskii]

> From: Daniel Martín <mardani29@yahoo.es>
> Cc: Michael Albinus <michael.albinus@gmx.de>,  emacs-devel@gnu.org
> Date: Sat, 12 Sep 2020 12:25:17 +0200
> 
> Stefan Kangas <stefan@marxist.se> writes:
> >
> > The idea is to avoid that it will take a very long time to run the unit
> > tests as the test suite grows.  Arguably a unit test should never take
> > longer than a second, and even that is in the slow end.  If we have
> > 10.000 unit tests taking a second each, just do the math of how long it
> > will take to run (even with parallelization).  We should not postpone
> > working on this until we are in that situation, IMHO, because by then it
> > will be a major pain.
> 
> I agree. We should differentiate between unit tests (they run in under a
> second) and end-to-end tests, which are slower but still valuable
> because they cover whole scenarios that unit tests can't cover.

Tramp tests need more time because they involve a remote system.
Moreover, the time taken by each Tramp test depends on the speed of
the connection, which you cannot know in advance.

So I conclude that the "under one second" criterion is not smart
enough to be applicable to Tramp tests, and maybe to some others as
well.

I also don't understand the more general issue with how long the test
suite runs.  While it runs, you can do something useful on a modern
system (or just go for a coffee), so why does it matter?  I've seen
test suites that take a very long time to run (e.g., look at Texinfo
or at Guile), and I never felt I was wasting my time by running those
comprehensive test suites.

I think we are exaggerating the importance of the time it takes to run
the tests.

> > I think the solution for important tests is to refactor the code to make
> > them run faster, for example by avoiding I/O or to make timers trigger
> > immediately.
> 
> Yes, I think this is a good approach to follow in general.

I don't, not in general.  Artificially making such changes will run a
risk of missing real problems, because the test runs in an environment
different from a real one.  This approach can be a good idea in some
cases, but in general we should try to run each test as close to
real-life conditions as possible.

Re: master 262d0c6: Mark some tests as expensive

[Michael Albinus]

Stefan Kangas <stefan@marxist.se> writes:

> Hi Michael,

Hi Stefan,

>> Could you please give a reasoning? Declaring tests as expansive decreases
>> heavily their application, giving us less chances to detect errors.
>
> These tests all took 5-60 seconds to run, most in the lower end
> admittedly.

With your change, you have tagged *all* autorevert tests as
:expensive-test. In practice this means, they won't run by default ever,
because people call "make check" only. Any error there, which is not
related to GNU/Linux, will be hidden (hydra and emba run "make
check-expensive", but only for GNU/Linux).

I don't think this is your intention.

> The idea is to avoid that it will take a very long time to run the unit
> tests as the test suite grows.  Arguably a unit test should never take
> longer than a second, and even that is in the slow end.  If we have
> 10.000 unit tests taking a second each, just do the math of how long it
> will take to run (even with parallelization).  We should not postpone
> working on this until we are in that situation, IMHO, because by then it
> will be a major pain.

There have been discussions about how long tests shall run. But there
hasn't been ever a conclusion to limit them to 1 second. Please discuss
this first, before making such changes.

> Best regards,
> Stefan Kangas

Best regards, Michael.

Re: master 262d0c6: Mark some tests as expensive

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

> I also don't understand the more general issue with how long the test
> suite runs.  While it runs, you can do something useful on a modern
> system (or just go for a coffee), so why does it matter?

I appreciate a fast test suite -- I often do a "make check" before
pushing just to see that nothing was inadvertently broken by a
seemingly-innocuous change.

So I think Stefan K's changes here are correct, and that we should have
small, fast unit tests only in the default "make check", and have the
longer-running integration tests on a different level.  (Hydra should
run all these, though, and if it doesn't, that should be changed.)

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: master 262d0c6: Mark some tests as expensive

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: Daniel Martín <mardani29@yahoo.es>,
>   michael.albinus@gmx.de,
>   stefan@marxist.se,  emacs-devel@gnu.org
> Date: Sat, 12 Sep 2020 13:15:06 +0200
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > I also don't understand the more general issue with how long the test
> > suite runs.  While it runs, you can do something useful on a modern
> > system (or just go for a coffee), so why does it matter?
> 
> I appreciate a fast test suite -- I often do a "make check" before
> pushing just to see that nothing was inadvertently broken by a
> seemingly-innocuous change.

How will that work if the change was in Tramp, or was related to
Tramp?  In my case, for example, it would most probably mean none of
the Tramp tests will ever run at all, because they all take more than
1 sec down here.

Also, there are some tests with inherent delays (auto-revert tests are
one example, but there are others) -- would that mean we don't run
these, either?

What I do for a "fast test" use case is to run only the tests directly
related to the change, usually the FOO-tests.el tests that correspond
to the file FOO I've changed.  Sometimes, there are related tests in
other files as well.  A simple "make FOO-tests" command is all that's
needed.

IOW, I don't think the time to run the tests is a good criterion, at
least not if applied globally to the entire suite.

Re: master 262d0c6: Mark some tests as expensive

[Michael Albinus]

Lars Ingebrigtsen <larsi@gnus.org> writes:

>> I also don't understand the more general issue with how long the test
>> suite runs.  While it runs, you can do something useful on a modern
>> system (or just go for a coffee), so why does it matter?
>
> I appreciate a fast test suite -- I often do a "make check" before
> pushing just to see that nothing was inadvertently broken by a
> seemingly-innocuous change.
>
> So I think Stefan K's changes here are correct, and that we should have
> small, fast unit tests only in the default "make check", and have the
> longer-running integration tests on a different level.  (Hydra should
> run all these, though, and if it doesn't, that should be changed.)

I would agree to move even more checks to :expensive, if we could run
them for all major target platforms. But we have only hydra and emba,
running on GNU/Linux.

Additionally, if we move *all* tests for a given feature to expensive,
as Stefan did with autorevert, your "make check" before pushing won't
help anymore for this feature.

Best regards, Michael.

Re: master 262d0c6: Mark some tests as expensive

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

> How will that work if the change was in Tramp, or was related to
> Tramp?  In my case, for example, it would most probably mean none of
> the Tramp tests will ever run at all, because they all take more than
> 1 sec down here.
>
> Also, there are some tests with inherent delays (auto-revert tests are
> one example, but there are others) -- would that mean we don't run
> these, either?

Yes, if they take more than a second to run.

We've already made this decision with all the tests previously marked as
expensive, so there's nothing new here.  We are formalising what counts
as a "too slow" test for the default "make check" case.

We could add more levels -- we now only have "normal" and "expensive",
right?  We could add an in-between level, "quite-expensive" or whatever.

> What I do for a "fast test" use case is to run only the tests directly
> related to the change, usually the FOO-tests.el tests that correspond
> to the file FOO I've changed.  Sometimes, there are related tests in
> other files as well.  A simple "make FOO-tests" command is all that's
> needed.

I do that, too, when working on something specific.  But being able to
say "make check" gives me greater confidence that I've not messed up
something in a related area (and let's face it, the entirety of Emacs is
"a related area" :-)).

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: master 262d0c6: Mark some tests as expensive

[Lars Ingebrigtsen]

Michael Albinus <michael.albinus@gmx.de> writes:

> I would agree to move even more checks to :expensive, if we could run
> them for all major target platforms. But we have only hydra and emba,
> running on GNU/Linux.

Yes, that sucks.  Are there political reasons not to add Macos and
Windows CI builds, or is it just because nobody has been bothered to?

On my build machine here at home, I've been installing VMs for several
Macos versions, as well as Open/Net/Freebsd, and I'm worryingly now
looking at installing a Windows VM, too.

It's not, like, difficult to do or anything...

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: master 262d0c6: Mark some tests as expensive

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: mardani29@yahoo.es,  michael.albinus@gmx.de,  stefan@marxist.se,
>   emacs-devel@gnu.org
> Date: Sat, 12 Sep 2020 14:11:40 +0200
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > How will that work if the change was in Tramp, or was related to
> > Tramp?  In my case, for example, it would most probably mean none of
> > the Tramp tests will ever run at all, because they all take more than
> > 1 sec down here.
> >
> > Also, there are some tests with inherent delays (auto-revert tests are
> > one example, but there are others) -- would that mean we don't run
> > these, either?
> 
> Yes, if they take more than a second to run.

So how do we make sure some change didn't introduce a bug in those
features?

> We've already made this decision with all the tests previously marked as
> expensive, so there's nothing new here.

There's a large gap between what is currently marked as "expensive"
tests and having entire packages not tested at all.  The latter sounds
too radical to me.  E.g., auto-revert is an important feature, used by
many people.  Not having it in regression testing sounds like a step
backward to me.

> > What I do for a "fast test" use case is to run only the tests directly
> > related to the change, usually the FOO-tests.el tests that correspond
> > to the file FOO I've changed.  Sometimes, there are related tests in
> > other files as well.  A simple "make FOO-tests" command is all that's
> > needed.
> 
> I do that, too, when working on something specific.  But being able to
> say "make check" gives me greater confidence that I've not messed up
> something in a related area (and let's face it, the entirety of Emacs is
> "a related area" :-)).

I'm talking about a balance here.  Losing the tests of complete
features just because we want tests to finish quickly sounds
sub-optimal to me.  Can we make a smarter balance?  After all, what
matters is not how long a single test runs, but how long the entire
suite runs.  Having a few tests that take more than a second doesn't
necessarily enlarge the total time significantly, especially since
quite a few of the tests are much shorter.  So maybe a better
criterion would be the time it takes to run the entire suite?

Re: master 262d0c6: Mark some tests as expensive

[Michael Albinus]

Lars Ingebrigtsen <larsi@gnus.org> writes:

Hi Lars,

>> I would agree to move even more checks to :expensive, if we could run
>> them for all major target platforms. But we have only hydra and emba,
>> running on GNU/Linux.
>
> Yes, that sucks.  Are there political reasons not to add Macos and
> Windows CI builds, or is it just because nobody has been bothered to?

I cannot speak for hydra. On emba, I don't know political restrictions;
it's rather that nobody has done it. Emba is a giitlab instance; if
somebody could point me where to get proper docker containers based on
*BSD, MacOS and Windows, we could try it.

> On my build machine here at home, I've been installing VMs for several
> Macos versions, as well as Open/Net/Freebsd, and I'm worryingly now
> looking at installing a Windows VM, too.
>
> It's not, like, difficult to do or anything...

I've tried to install a MacOS VM on my laptop, and I failed. Could you
pls point me to a proper starting point (web page, I guess)?

Best regards, Michael.

Re: master 262d0c6: Mark some tests as expensive

[Lars Ingebrigtsen]

Michael Albinus <michael.albinus@gmx.de> writes:

> I've tried to install a MacOS VM on my laptop, and I failed. Could you
> pls point me to a proper starting point (web page, I guess)?

I used this to install the VM:

  https://github.com/foxlet/macOS-Simple-KVM

And then this is my recipe for building Emacs on Macos:

  https://lars.ingebrigtsen.no/2020/08/02/emacs-on-macos-for-linux-peeps/

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: master 262d0c6: Mark some tests as expensive

[Dmitry Gutov]

On 12.09.2020 15:30, Michael Albinus wrote:
>> Yes, that sucks.  Are there political reasons not to add Macos and
>> Windows CI builds, or is it just because nobody has been bothered to?
> I cannot speak for hydra. On emba, I don't know political restrictions;
> it's rather that nobody has done it. Emba is a giitlab instance; if
> somebody could point me where to get proper docker containers based on
> *BSD, MacOS and Windows, we could try it.

I doubt anyone is going to buy a Windows license for this, but we could ask.

As far as MacOS, though, it's impossible to do legally because its 
license prohibits running the OS on anything but Apple hardware. There 
are some technical measures enforcing that too, as I heard.

Re: master 262d0c6: Mark some tests as expensive

[Daniel Martín]

Eli Zaretskii <eliz@gnu.org> writes:
>
> Tramp tests need more time because they involve a remote system.
> Moreover, the time taken by each Tramp test depends on the speed of
> the connection, which you cannot know in advance.

If they depend on the connection speed, or if a host is online or
offline, they also introduce non-determinism that perhaps is not
apparent now, but can cause problems when the Tramp codebase (and its
number of tests) scale. For example, if a Tramp test has a 0.1% chance
of failure because of an unrelated network problem, then if the Tramp
test suite reaches a point where 10000 tests are run per day, people
would be investigating 10 test flakes per day. That's a good reason for
people to lose confidence in the Tramp test suite and ignore failures.

One could argue that the Tramp test suite will never reach such a high
number of tests/contributions, and maybe that's fine, but the problem of
scale exists, IMHO.

>
> I don't, not in general.  Artificially making such changes will run a
> risk of missing real problems, because the test runs in an environment
> different from a real one.  This approach can be a good idea in some
> cases, but in general we should try to run each test as close to
> real-life conditions as possible.

I'd say "we should try to run *some* tests as close to real-life
conditions as possible". By abstracting the environment in some tests,
one could potentially test an infinite number of environments and error
conditions, not only what the test happens to run on. We would still
have a few end-to-end tests that check that the program as a whole works
fine, of course. The trade-off is that writing that kind of hermetic
tests takes more time, specially for packages like Tramp.

Having said that, I think that covering Emacs functionality with an
end-to-end test is much better than having no test at all. But I'd
expect some of those end-to-end tests marked as ":expensive-test" to
evolve into smaller, more hermetic ones, as the test suite matures.

Re: master 262d0c6: Mark some tests as expensive

[Eli Zaretskii]

> From: Daniel Martín <mardani29@yahoo.es>
> Cc: stefan@marxist.se,  michael.albinus@gmx.de,  emacs-devel@gnu.org
> Date: Sat, 12 Sep 2020 16:00:00 +0200
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> >
> > Tramp tests need more time because they involve a remote system.
> > Moreover, the time taken by each Tramp test depends on the speed of
> > the connection, which you cannot know in advance.
> 
> If they depend on the connection speed, or if a host is online or
> offline, they also introduce non-determinism that perhaps is not
> apparent now, but can cause problems when the Tramp codebase (and its
> number of tests) scale. For example, if a Tramp test has a 0.1% chance
> of failure because of an unrelated network problem, then if the Tramp
> test suite reaches a point where 10000 tests are run per day, people
> would be investigating 10 test flakes per day. That's a good reason for
> people to lose confidence in the Tramp test suite and ignore failures.

I don't understand what you are arguing here.  AFAIK, Michael already
either marked "unstable" or rewrote any tests that are known to suffer
from non-determinism.  Slow speed doesn't necessarily add
non-determinism, not if Emacs waits for the operation to complete.

So I don't see how this is relevant to the issue at hand, which is the
criteria for marking tests "expensive" and not running them by
default.  What am I missing?

Re: master 262d0c6: Mark some tests as expensive

[Daniel Martín]

Dmitry Gutov <dgutov@yandex.ru> writes:
>
> As far as MacOS, though, it's impossible to do legally because its
> license prohibits running the OS on anything but Apple hardware. There 
> are some technical measures enforcing that too, as I heard.

Correct, the license only permits virtualizing macOS on Apple hardware.

There are cloud providers like MacStadium that provide virtualized macOS
environments that can integrate with Gitlab, but they are costly and
probably out of the question for the GNU project.

Re: master 262d0c6: Mark some tests as expensive

[Michael Albinus]

Daniel Martín <mardani29@yahoo.es> writes:

Hi Daniel,

> Eli Zaretskii <eliz@gnu.org> writes:
>>
>> Tramp tests need more time because they involve a remote system.
>> Moreover, the time taken by each Tramp test depends on the speed of
>> the connection, which you cannot know in advance.
>
> If they depend on the connection speed, or if a host is online or
> offline, they also introduce non-determinism that perhaps is not
> apparent now, but can cause problems when the Tramp codebase (and its
> number of tests) scale. For example, if a Tramp test has a 0.1% chance
> of failure because of an unrelated network problem, then if the Tramp
> test suite reaches a point where 10000 tests are run per day, people
> would be investigating 10 test flakes per day. That's a good reason for
> people to lose confidence in the Tramp test suite and ignore failures.

Tramp tests don't need a remote connection by default. They simulate a
connection by a "mock" method, which is in fact a local shell.

Real remote connections can be tested also, but this doesn't happen with
"make check". Read the Commentary section of tramp-tests.el for details.

> I'd say "we should try to run *some* tests as close to real-life
> conditions as possible". By abstracting the environment in some tests,
> one could potentially test an infinite number of environments and error
> conditions, not only what the test happens to run on. We would still
> have a few end-to-end tests that check that the program as a whole works
> fine, of course. The trade-off is that writing that kind of hermetic
> tests takes more time, specially for packages like Tramp.

For the records, I keep an ansible script which runs tramp-tests.el in
~75 different configurations, all of them using real remote hosts. I run
it at least prior releasing a new Tramp version; it takes up to 2 days
(the longest single run of tramp-tests.el takes about 10 hours). This
won't go into Emacs' "make check", of course.

Best regards, Michael.

Re: master 262d0c6: Mark some tests as expensive

[Michael Albinus]

Dmitry Gutov <dgutov@yandex.ru> writes:

> I doubt anyone is going to buy a Windows license for this, but we could ask.

There is the (30? 90? days) free trial MS Windows. Couldn't we set up it
again and again automatically for the tests, say on emba?

Best regards, Michael.

Re: master 262d0c6: Mark some tests as expensive

[Lars Ingebrigtsen]

Dmitry Gutov <dgutov@yandex.ru> writes:

> I doubt anyone is going to buy a Windows license for this, but we could ask.

My impression is that Microsoft is quite developer friendly, and if you
ask them, you get access to their entire development er site, where you
can download ISOs for any Windows version.  I forget what the program is
called.

Many free software have accounts with Microsoft.

> As far as MacOS, though, it's impossible to do legally because its
> license prohibits running the OS on anything but Apple hardware. There
> are some technical measures enforcing that too, as I heard.

There are license problem, but no technical measures, really.  Macos
runs fine under qemu on Linux.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: master 262d0c6: Mark some tests as expensive

[Daniel Martín]

Michael Albinus <michael.albinus@gmx.de> writes:
>
> Tramp tests don't need a remote connection by default. They simulate a
> connection by a "mock" method, which is in fact a local shell.

Thanks for the clarification. The sentence I quoted made me think those
tests were accessing real hosts using the network.

Re: master 262d0c6: Mark some tests as expensive

[Daniel Martín]

Eli Zaretskii <eliz@gnu.org> writes:
>
> I don't understand what you are arguing here.  AFAIK, Michael already
> either marked "unstable" or rewrote any tests that are known to suffer
> from non-determinism.  Slow speed doesn't necessarily add
> non-determinism, not if Emacs waits for the operation to complete.
>
> So I don't see how this is relevant to the issue at hand, which is the
> criteria for marking tests "expensive" and not running them by
> default.  What am I missing?

After reading "Tramp tests need more time because they involve a remote
system" I assumed that those tests were accessing real hosts using the
network. That's why I mentioned the importance of determinism in
tests. Michael has pointed out that network connections are correctly
"mocked" in "make check", which seems fine.

Re: master 262d0c6: Mark some tests as expensive

[Dmitry Gutov]

On 12.09.2020 17:49, Michael Albinus wrote:
> Dmitry Gutov <dgutov@yandex.ru> writes:
> 
>> I doubt anyone is going to buy a Windows license for this, but we could ask.
> 
> There is the (30? 90? days) free trial MS Windows. Couldn't we set up it
> again and again automatically for the tests, say on emba?

I'm not sure about the legality of this, but yeah, it could work.

Especially since Microsoft itself, IIRC, distributed (also time-limited) 
images with different versions of Internet Explorer for testing.

Re: master 262d0c6: Mark some tests as expensive

[Michael Albinus]

Lars Ingebrigtsen <larsi@gnus.org> writes:

Hi Lars,

> We've already made this decision with all the tests previously marked as
> expensive, so there's nothing new here.  We are formalising what counts
> as a "too slow" test for the default "make check" case.

When I have marked tests as expensive, I've always checked that the test
isn't needed for the basic feature, and whether the test runs long. I
believe an expensive test must be evaluated for both criteria.

Best regards, Michael.

Re: master 262d0c6: Mark some tests as expensive

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

>> Yes, if they take more than a second to run.
>
> So how do we make sure some change didn't introduce a bug in those
> features?

We either say "make check-expensive", or we live on in uncertainty, as
with all those other functions we've marked as expensive.

>> We've already made this decision with all the tests previously marked as
>> expensive, so there's nothing new here.
>
> There's a large gap between what is currently marked as "expensive"
> tests and having entire packages not tested at all.  The latter sounds
> too radical to me.  E.g., auto-revert is an important feature, used by
> many people.  Not having it in regression testing sounds like a step
> backward to me.

It's still being tested -- just not as often.

> I'm talking about a balance here.  Losing the tests of complete
> features just because we want tests to finish quickly sounds
> sub-optimal to me.  Can we make a smarter balance?

Sure, there's a balance, and I think the current balance is a bit too
skewed towards having too many slow tests in the "make check".  I think
that, ideally, "make check" should be so fast that people run it as
their standard workflow before pushing a change, and we're not quite
there.

Having people do "make check" as a matter of routine, and running 97% of
the tests is, in my opinion, better than people doing a "make check"
seldom, but running 98% of the tests.

Where the cutoff is a matter of balance, yes, but I think a test that
takes a second is way too slow to be run in the routine case.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: master 262d0c6: Mark some tests as expensive

[Lars Ingebrigtsen]

Michael Albinus <michael.albinus@gmx.de> writes:

> When I have marked tests as expensive, I've always checked that the test
> isn't needed for the basic feature, and whether the test runs long. I
> believe an expensive test must be evaluated for both criteria.

Hm...  I guess that sounds reasonable, but I don't agree.  :-)  The only
feature that is so basic that it has to run, no matter how long it
takes, is "does Emacs start?", and that test is a lot faster than a
second.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: master 262d0c6: Mark some tests as expensive

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Date: Sun, 13 Sep 2020 14:33:21 +0200
> Cc: Eli Zaretskii <eliz@gnu.org>, mardani29@yahoo.es, stefan@marxist.se,
>  emacs-devel@gnu.org
> 
> Michael Albinus <michael.albinus@gmx.de> writes:
> 
> > When I have marked tests as expensive, I've always checked that the test
> > isn't needed for the basic feature, and whether the test runs long. I
> > believe an expensive test must be evaluated for both criteria.
> 
> Hm...  I guess that sounds reasonable, but I don't agree.  :-)  The only
> feature that is so basic that it has to run, no matter how long it
> takes, is "does Emacs start?", and that test is a lot faster than a
> second.

I think that's a slippery slope.  We will very quickly end up with
many bugs that aren't uncovered in time.

Re: master 262d0c6: Mark some tests as expensive

[Stefan Monnier]

> Having people do "make check" as a matter of routine, and running 97% of
> the tests is, in my opinion, better than people doing a "make check"
> seldom, but running 98% of the tests.
>
> Where the cutoff is a matter of balance, yes, but I think a test that
> takes a second is way too slow to be run in the routine case.

FWIW, I think The Right Solution is to label each test with its "cost"
(which could be auto-computed by a script that runs those tests to
measure that cost).  Then we can provide a command to run all tests
cheaper than X, and the end-user gets to decide where is "the cutoff" in
his balance.


        Stefan

Re: master 262d0c6: Mark some tests as expensive

[Lars Ingebrigtsen]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

> FWIW, I think The Right Solution is to label each test with its "cost"
> (which could be auto-computed by a script that runs those tests to
> measure that cost).  Then we can provide a command to run all tests
> cheaper than X, and the end-user gets to decide where is "the cutoff" in
> his balance.

Sounds great.  Would we put the "cost" into the tests themselves, or
have a separate (auto-generated) file for this?

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: master 262d0c6: Mark some tests as expensive

[Michael Albinus]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> Having people do "make check" as a matter of routine, and running 97% of
>> the tests is, in my opinion, better than people doing a "make check"
>> seldom, but running 98% of the tests.
>>
>> Where the cutoff is a matter of balance, yes, but I think a test that
>> takes a second is way too slow to be run in the routine case.
>
> FWIW, I think The Right Solution is to label each test with its "cost"
> (which could be auto-computed by a script that runs those tests to
> measure that cost).  Then we can provide a command to run all tests
> cheaper than X, and the end-user gets to decide where is "the cutoff" in
> his balance.

Nice idea. The problem will be to decide what are the default costs we
accept running "make check". I fear that there will be voices to make
the default costs such low, that (for example) auto-revert tests are
kicked off. As we have now.

>         Stefan

Best regards, Michael.

Re: master 262d0c6: Mark some tests as expensive

[Stefan Kangas]

Michael Albinus <michael.albinus@gmx.de> writes:

> With your change, you have tagged *all* autorevert tests as
> :expensive-test. In practice this means, they won't run by default ever,
> because people call "make check" only. Any error there, which is not
> related to GNU/Linux, will be hidden (hydra and emba run "make
> check-expensive", but only for GNU/Linux).
>
> I don't think this is your intention.

Sorry for the delayed reply here.

It does seem unfortunate that no tests run for autorevert-mode in the
default case.  I have a patch in the works that will reduce the running
time for these tests, I will send it here when it's done.  (Basically
the patch would temporarily lower auto-revert-interval, remove sleeps
and manually set mtime to lower values than the current time.)

> There have been discussions about how long tests shall run. But there
> hasn't been ever a conclusion to limit them to 1 second. Please discuss
> this first, before making such changes.

Sure, I just didn't expect this to be controversial.  Sorry if this
change caused any problems.

Best regards,
Stefan Kangas

Re: master 262d0c6: Mark some tests as expensive

[Michael Albinus]

Stefan Kangas <stefan@marxist.se> writes:

Hi Stefan,

> It does seem unfortunate that no tests run for autorevert-mode in the
> default case.  I have a patch in the works that will reduce the running
> time for these tests, I will send it here when it's done.  (Basically
> the patch would temporarily lower auto-revert-interval, remove sleeps
> and manually set mtime to lower values than the current time.)

Would be helpful. However, there have been discussions (and changes) of
the timeouts in the past, so I'm not sure we can reduce them w/o causing
damage. Please check the git history of autorevert-tests.el for such
changes, hopefully there are bug numbers in the git log you could consult
for the respective discussions. Otherwise, a search on the emacs-devel
and emacs-bugs MLs for the given timeframe of such a change might be helpful.

>> There have been discussions about how long tests shall run. But there
>> hasn't been ever a conclusion to limit them to 1 second. Please discuss
>> this first, before making such changes.
>
> Sure, I just didn't expect this to be controversial.  Sorry if this
> change caused any problems.

IIRC, this discussion happened before you've started with work on
Emacs. I'm pretty sure there's something in the emacs-devel ML, but this
was years ago.

> Best regards,
> Stefan Kangas

Best regards, Michael.

Re: master 262d0c6: Mark some tests as expensive

[Stefan Kangas]

[-- Attachment #1: Type: text/plain, Size: 1986 bytes --]

Hi Michael,

Michael Albinus <michael.albinus@gmx.de> writes:

>> It does seem unfortunate that no tests run for autorevert-mode in the
>> default case.  I have a patch in the works that will reduce the running
>> time for these tests, I will send it here when it's done.  (Basically
>> the patch would temporarily lower auto-revert-interval, remove sleeps
>> and manually set mtime to lower values than the current time.)
>
> Would be helpful. However, there have been discussions (and changes) of
> the timeouts in the past, so I'm not sure we can reduce them w/o causing
> damage. Please check the git history of autorevert-tests.el for such
> changes, hopefully there are bug numbers in the git log you could consult
> for the respective discussions. Otherwise, a search on the emacs-devel
> and emacs-bugs MLs for the given timeframe of such a change might be helpful.

So I have attached here a patch that employs the above mentioned
techniques somewhat aggressively to reduce the running time as follows:

Ran 7 tests, 7 results as expected, 0 unexpected (2020-10-18
19:38:29+0200, 2.323729 sec)

Everything works fine here, but I don't know if any of this would cause
any problems elsewhere.  One idea is to just push it and see what
breaks, and then adapt accordingly.

Please let me know what you think.

(Note that the patch takes care to not re-indent any of the tests in
order to be easier to review.)

---

Also, I've had some trouble finding the past discussions about this.
I've been looking for "auto-revert-tests", "autorevert-tests",
"autorevert-tests.el" and "auto-revert-interval" in both emacs-devel and
bug-gnu-emacs.  The only bugs I was able to dig up was:

https://debbugs.gnu.org/cgi/bugreport.cgi?bug=21668
https://debbugs.gnu.org/cgi/bugreport.cgi?bug=32645
https://debbugs.gnu.org/cgi/bugreport.cgi?bug=35418

But I'm not sure if any of that is even relevant here.  Let me know if
you have any ideas for what I could look for.

Best regards,
Stefan Kangas

[-- Attachment #2: 0001-Make-auto-revert-mode-tests-run-faster.patch --]
[-- Type: text/x-diff, Size: 18426 bytes --]

From 877dfbfc1381e1865f36ae9c15b3a9e99b6c5699 Mon Sep 17 00:00:00 2001
From: Stefan Kangas <stefan@marxist.se>
Date: Sat, 12 Sep 2020 19:49:20 +0200
Subject: [PATCH] Make auto-revert-mode tests run faster

* test/lisp/autorevert-tests.el (auto-revert--timeout): Make into
defun and shorten timeout by a factor 10.
(auto-revert--wait-for-revert): Cut timeouts in half.
(with-auto-revert-test): New macro to set timeout to 0.1.
(auto-revert-tests--write-file): New defun.
(auto-revert-test00-auto-revert-mode)
(auto-revert-test01-auto-revert-several-files)
(auto-revert-test02-auto-revert-deleted-file)
(auto-revert-test03-auto-revert-tail-mode)
(auto-revert-test04-auto-revert-mode-dired)
(auto-revert-test05-global-notify)
(auto-revert-test06-write-file): Adapt test to run faster.
---
 test/lisp/autorevert-tests.el | 124 +++++++++++++++++-----------------
 1 file changed, 62 insertions(+), 62 deletions(-)

diff --git a/test/lisp/autorevert-tests.el b/test/lisp/autorevert-tests.el
index 3243a80e52..a90fa69f1b 100644
--- a/test/lisp/autorevert-tests.el
+++ b/test/lisp/autorevert-tests.el
@@ -61,8 +61,9 @@
       file-notify-debug nil
       tramp-verbose 0)
 
-(defconst auto-revert--timeout (1+ auto-revert-interval)
-  "Time to wait for a message.")
+(defun auto-revert--timeout ()
+  "Time to wait for a message."
+  (+ auto-revert-interval 0.1))
 
 (defvar auto-revert--messages nil
   "Used to collect messages issued during a section of a test.")
@@ -125,14 +126,14 @@ auto-revert--wait-for-revert
   ;; Remote files do not cooperate well with timers.  So we count ourselves.
   (let ((ct (current-time)))
     (while (and (< (float-time (time-subtract (current-time) ct))
-                   auto-revert--timeout)
+                   (auto-revert--timeout))
                 (null (string-match
                        (format-message
                         "Reverting buffer `%s'\\." (buffer-name buffer))
                        auto-revert--messages)))
       (if (with-current-buffer buffer auto-revert-use-notify)
-          (read-event nil nil 0.1)
-        (sleep-for 0.1)))))
+          (read-event nil nil 0.05)
+        (sleep-for 0.05)))))
 
 (defmacro auto-revert--deftest-remote (test docstring)
   "Define ert `TEST-remote' for remote files."
@@ -152,16 +153,29 @@ auto-revert--deftest-remote
            (funcall (ert-test-body ert-test))
          (error (message "%s" err) (signal (car err) (cdr err)))))))
 
+(defmacro with-auto-revert-test (&rest body)
+  `(let ((auto-revert-interval-orig auto-revert-interval))
+     (unwind-protect
+         (progn
+           (customize-set-variable 'auto-revert-interval 0.1)
+           ,@body)
+       (customize-set-variable 'auto-revert-interval auto-revert-interval-orig))))
+
+(defun auto-revert-tests--write-file (text file time-delta &optional append)
+  (write-region text nil file append 'no-message)
+  (set-file-times file (time-subtract (current-time) time-delta)))
+
 (ert-deftest auto-revert-test00-auto-revert-mode ()
   "Check autorevert for a file."
   ;; `auto-revert-buffers' runs every 5".  And we must wait, until the
   ;; file has been reverted.
-  :tags '(:expensive-test)
+  (with-auto-revert-test
   (let ((tmpfile (make-temp-file "auto-revert-test"))
+        (times '(60 30 15))
         buf)
     (unwind-protect
-	(progn
-          (write-region "any text" nil tmpfile nil 'no-message)
+        (progn
+          (auto-revert-tests--write-file "any text" tmpfile (pop times))
 	  (setq buf (find-file-noselect tmpfile))
           (with-current-buffer buf
             (ert-with-message-capture auto-revert--messages
@@ -169,14 +183,12 @@ auto-revert-test00-auto-revert-mode
               ;; `buffer-stale--default-function' checks for
               ;; `verify-visited-file-modtime'.  We must ensure that it
               ;; returns nil.
-              (sleep-for 1)
               (auto-revert-mode 1)
               (should auto-revert-mode)
 
-              ;; Modify file.  We wait for a second, in order to have
+              ;;  order to have
               ;; another timestamp.
-              (sleep-for 1)
-              (write-region "another text" nil tmpfile nil 'no-message)
+              (auto-revert-tests--write-file "another text" tmpfile (pop times))
 
               ;; Check, that the buffer has been reverted.
               (auto-revert--wait-for-revert buf))
@@ -185,8 +197,7 @@ auto-revert-test00-auto-revert-mode
             ;; When the buffer is modified, it shall not be reverted.
             (ert-with-message-capture auto-revert--messages
               (set-buffer-modified-p t)
-              (sleep-for 1)
-              (write-region "any text" nil tmpfile nil 'no-message)
+              (auto-revert-tests--write-file "any text" tmpfile (pop times))
 
               ;; Check, that the buffer hasn't been reverted.
               (auto-revert--wait-for-revert buf))
@@ -196,7 +207,7 @@ auto-revert-test00-auto-revert-mode
       (ignore-errors
         (with-current-buffer buf (set-buffer-modified-p nil))
         (kill-buffer buf))
-      (ignore-errors (delete-file tmpfile)))))
+      (ignore-errors (delete-file tmpfile))))))
 
 (auto-revert--deftest-remote auto-revert-test00-auto-revert-mode
   "Check autorevert for a remote file.")
@@ -204,9 +215,9 @@ auto-revert-test00-auto-revert-mode
 ;; This is inspired by Bug#21841.
 (ert-deftest auto-revert-test01-auto-revert-several-files ()
   "Check autorevert for several files at once."
-  :tags '(:expensive-test)
   (skip-unless (executable-find "cp" (file-remote-p temporary-file-directory)))
 
+  (with-auto-revert-test
   (let* ((cp (executable-find "cp" (file-remote-p temporary-file-directory)))
          (tmpdir1 (make-temp-file "auto-revert-test" 'dir))
          (tmpdir2 (make-temp-file "auto-revert-test" 'dir))
@@ -214,12 +225,13 @@ auto-revert-test01-auto-revert-several-files
           (make-temp-file (expand-file-name "auto-revert-test" tmpdir1)))
          (tmpfile2
           (make-temp-file (expand-file-name "auto-revert-test" tmpdir1)))
+         (times '(120 60 30 15))
          buf1 buf2)
     (unwind-protect
         (ert-with-message-capture auto-revert--messages
-          (write-region "any text" nil tmpfile1 nil 'no-message)
+          (auto-revert-tests--write-file "any text" tmpfile1 (pop times))
           (setq buf1 (find-file-noselect tmpfile1))
-          (write-region "any text" nil tmpfile2 nil 'no-message)
+          (auto-revert-tests--write-file "any text" tmpfile2 (pop times))
           (setq buf2 (find-file-noselect tmpfile2))
 
           (dolist (buf (list buf1 buf2))
@@ -228,21 +240,19 @@ auto-revert-test01-auto-revert-several-files
               ;; `buffer-stale--default-function' checks for
               ;; `verify-visited-file-modtime'.  We must ensure that
               ;; it returns nil.
-              (sleep-for 1)
               (auto-revert-mode 1)
               (should auto-revert-mode)))
 
           ;; Modify files.  We wait for a second, in order to have
           ;; another timestamp.
-          (sleep-for 1)
-          (write-region
-           "another text" nil
+          (auto-revert-tests--write-file
+           "another text"
            (expand-file-name (file-name-nondirectory tmpfile1) tmpdir2)
-           nil 'no-message)
-          (write-region
-           "another text" nil
+           (pop times))
+          (auto-revert-tests--write-file
+           "another text"
            (expand-file-name (file-name-nondirectory tmpfile2) tmpdir2)
-           nil 'no-message)
+           (pop times))
           ;;(copy-directory tmpdir2 tmpdir1 nil 'copy-contents)
           ;; Strange, that `copy-directory' does not work as expected.
           ;; The following shell command is not portable on all
@@ -263,7 +273,7 @@ auto-revert-test01-auto-revert-several-files
           (with-current-buffer buf (set-buffer-modified-p nil))
           (kill-buffer buf)))
       (ignore-errors (delete-directory tmpdir1 'recursive))
-      (ignore-errors (delete-directory tmpdir2 'recursive)))))
+      (ignore-errors (delete-directory tmpdir2 'recursive))))))
 
 (auto-revert--deftest-remote auto-revert-test01-auto-revert-several-files
   "Check autorevert for several remote files at once.")
@@ -271,19 +281,20 @@ auto-revert-test01-auto-revert-several-files
 ;; This is inspired by Bug#23276.
 (ert-deftest auto-revert-test02-auto-revert-deleted-file ()
   "Check autorevert for a deleted file."
-  :tags '(:expensive-test)
   ;; Repeated unpredictable failures, bug#32645.
   ;; Unlikely to be hydra-specific?
 ;  (skip-unless (not (getenv "EMACS_HYDRA_CI")))
 
+  (with-auto-revert-test
   (let ((tmpfile (make-temp-file "auto-revert-test"))
         ;; Try to catch bug#32645.
         (auto-revert-debug (getenv "EMACS_HYDRA_CI"))
         (file-notify-debug (getenv "EMACS_HYDRA_CI"))
+        (times '(120 60 30 15))
         buf desc)
     (unwind-protect
 	(progn
-          (write-region "any text" nil tmpfile nil 'no-message)
+          (auto-revert-tests--write-file "any text" tmpfile (pop times))
 	  (setq buf (find-file-noselect tmpfile))
 	  (with-current-buffer buf
             (should-not
@@ -292,7 +303,6 @@ auto-revert-test02-auto-revert-deleted-file
             ;; `buffer-stale--default-function' checks for
             ;; `verify-visited-file-modtime'.  We must ensure that
             ;; it returns nil.
-            (sleep-for 1)
             (auto-revert-mode 1)
             (should auto-revert-mode)
             (setq desc auto-revert-notify-watch-descriptor)
@@ -308,8 +318,7 @@ auto-revert-test02-auto-revert-deleted-file
              nil t)
 
             (ert-with-message-capture auto-revert--messages
-              (sleep-for 1)
-              (write-region "another text" nil tmpfile nil 'no-message)
+              (auto-revert-tests--write-file "another text" tmpfile (pop times))
               (auto-revert--wait-for-revert buf))
             ;; Check, that the buffer hasn't been reverted.  File
             ;; notification should be disabled, falling back to
@@ -325,8 +334,7 @@ auto-revert-test02-auto-revert-deleted-file
             ;; reverted.
             (kill-local-variable 'before-revert-hook)
             (ert-with-message-capture auto-revert--messages
-              (sleep-for 1)
-              (write-region "another text" nil tmpfile nil 'no-message)
+              (auto-revert-tests--write-file "another text" tmpfile (pop times))
               (auto-revert--wait-for-revert buf))
             ;; Check, that the buffer has been reverted.
             (should (string-match "another text" (buffer-string)))
@@ -338,8 +346,7 @@ auto-revert-test02-auto-revert-deleted-file
 
             ;; An empty file shall still be reverted.
             (ert-with-message-capture auto-revert--messages
-              (sleep-for 1)
-              (write-region "" nil tmpfile nil 'no-message)
+              (auto-revert-tests--write-file "" tmpfile (pop times))
               (auto-revert--wait-for-revert buf))
             ;; Check, that the buffer has been reverted.
             (should (string-equal "" (buffer-string)))))
@@ -348,7 +355,7 @@ auto-revert-test02-auto-revert-deleted-file
       (ignore-errors
         (with-current-buffer buf (set-buffer-modified-p nil))
         (kill-buffer buf))
-      (ignore-errors (delete-file tmpfile)))))
+      (ignore-errors (delete-file tmpfile))))))
 
 (auto-revert--deftest-remote auto-revert-test02-auto-revert-deleted-file
   "Check autorevert for a deleted remote file.")
@@ -357,28 +364,25 @@ auto-revert-test03-auto-revert-tail-mode
   "Check autorevert tail mode."
   ;; `auto-revert-buffers' runs every 5".  And we must wait, until the
   ;; file has been reverted.
-  :tags '(:expensive-test)
   (let ((tmpfile (make-temp-file "auto-revert-test"))
+        (times '(30 15))
         buf)
     (unwind-protect
         (ert-with-message-capture auto-revert--messages
-          (write-region "any text" nil tmpfile nil 'no-message)
+          (auto-revert-tests--write-file "any text" tmpfile (pop times))
 	  (setq buf (find-file-noselect tmpfile))
 	  (with-current-buffer buf
             ;; `buffer-stale--default-function' checks for
             ;; `verify-visited-file-modtime'.  We must ensure that it
             ;; returns nil.
-            (sleep-for 1)
-	    (auto-revert-tail-mode 1)
+            (auto-revert-tail-mode 1)
 	    (should auto-revert-tail-mode)
             (erase-buffer)
             (insert "modified text\n")
             (set-buffer-modified-p nil)
 
-	    ;; Modify file.  We wait for a second, in order to have
-	    ;; another timestamp.
-	    (sleep-for 1)
-            (write-region "another text" nil tmpfile 'append 'no-message)
+            ;; Modify file.
+            (auto-revert-tests--write-file "another text" tmpfile (pop times) 'append)
 
 	    ;; Check, that the buffer has been reverted.
             (auto-revert--wait-for-revert buf)
@@ -396,9 +400,10 @@ auto-revert-test04-auto-revert-mode-dired
   "Check autorevert for dired."
   ;; `auto-revert-buffers' runs every 5".  And we must wait, until the
   ;; file has been reverted.
-  :tags '(:expensive-test)
+  (with-auto-revert-test
   (let* ((tmpfile (make-temp-file "auto-revert-test"))
          (name (file-name-nondirectory tmpfile))
+         (times '(30))
          buf)
     (unwind-protect
 	(progn
@@ -407,16 +412,13 @@ auto-revert-test04-auto-revert-mode-dired
             ;; `buffer-stale--default-function' checks for
             ;; `verify-visited-file-modtime'.  We must ensure that it
             ;; returns nil.
-            (sleep-for 1)
             (auto-revert-mode 1)
             (should auto-revert-mode)
 	    (should
              (string-match name (substring-no-properties (buffer-string))))
 
             (ert-with-message-capture auto-revert--messages
-              ;; Delete file.  We wait for a second, in order to have
-              ;; another timestamp.
-              (sleep-for 1)
+              ;; Delete file.
               (delete-file tmpfile)
               (auto-revert--wait-for-revert buf))
             ;; Check, that the buffer has been reverted.
@@ -427,8 +429,7 @@ auto-revert-test04-auto-revert-mode-dired
               ;; Make dired buffer modified.  Check, that the buffer has
               ;; been still reverted.
               (set-buffer-modified-p t)
-              (sleep-for 1)
-              (write-region "any text" nil tmpfile nil 'no-message)
+              (auto-revert-tests--write-file "any text" tmpfile (pop times))
 
               (auto-revert--wait-for-revert buf))
             ;; Check, that the buffer has been reverted.
@@ -439,7 +440,7 @@ auto-revert-test04-auto-revert-mode-dired
       (ignore-errors
         (with-current-buffer buf (set-buffer-modified-p nil))
         (kill-buffer buf))
-      (ignore-errors (delete-file tmpfile)))))
+      (ignore-errors (delete-file tmpfile))))))
 
 (auto-revert--deftest-remote auto-revert-test04-auto-revert-mode-dired
   "Check remote autorevert for dired.")
@@ -468,9 +469,9 @@ auto-revert-test--wait-for-buffer-text
 
 (ert-deftest auto-revert-test05-global-notify ()
   "Test `global-auto-revert-mode' without polling."
-  :tags '(:expensive-test)
   (skip-unless (or file-notify--library
                    (file-remote-p temporary-file-directory)))
+  (with-auto-revert-test
   (let* ((auto-revert-use-notify t)
          (auto-revert-avoid-polling t)
          (was-in-global-auto-revert-mode global-auto-revert-mode)
@@ -510,7 +511,7 @@ auto-revert-test05-global-notify
           (auto-revert-test--wait-for
            (lambda () (buffer-local-value
                        'auto-revert-notify-watch-descriptor buf-3))
-           auto-revert--timeout)
+           (auto-revert--timeout))
           (should (buffer-local-value
                    'auto-revert-notify-watch-descriptor buf-3))
           (auto-revert-test--write-file "3-a" file-3)
@@ -519,11 +520,10 @@ auto-revert-test05-global-notify
 
           ;; Delete a visited file, and re-create it with new contents.
           (delete-file file-1)
-          (sleep-for 0.5)
           (should (equal (auto-revert-test--buffer-string buf-1) "1-a"))
           (auto-revert-test--write-file "1-b" file-1)
           (auto-revert-test--wait-for-buffer-text
-           buf-1 "1-b" auto-revert--timeout)
+           buf-1 "1-b" (auto-revert--timeout))
           (should (buffer-local-value
                    'auto-revert-notify-watch-descriptor buf-1))
 
@@ -533,7 +533,7 @@ auto-revert-test05-global-notify
           (should (equal (auto-revert-test--buffer-string buf-2) "2-a"))
           (auto-revert-test--write-file "2-b" file-2b)
           (auto-revert-test--wait-for-buffer-text
-           buf-2 "2-b" auto-revert--timeout)
+           buf-2 "2-b" (auto-revert--timeout))
           (should (buffer-local-value
                    'auto-revert-notify-watch-descriptor buf-2)))
 
@@ -544,16 +544,16 @@ auto-revert-test05-global-notify
         (ignore-errors (kill-buffer buf)))
       (dolist (file (list file-1 file-2 file-2b file-3))
         (ignore-errors (delete-file file)))
-      )))
+      ))))
 
 (auto-revert--deftest-remote auto-revert-test05-global-notify
   "Test `global-auto-revert-mode' without polling for remote buffers.")
 
 (ert-deftest auto-revert-test06-write-file ()
   "Verify that notification follows `write-file' correctly."
-  :tags '(:expensive-test)
   (skip-unless (or file-notify--library
                    (file-remote-p temporary-file-directory)))
+  (with-auto-revert-test
   (let* ((auto-revert-use-notify t)
          (file-1 (make-temp-file "auto-revert-test"))
          (file-2 (concat file-1 "-2"))
@@ -572,13 +572,13 @@ auto-revert-test06-write-file
 
             (auto-revert-test--write-file "C" file-2)
             (auto-revert-test--wait-for-buffer-text
-             buf "C" auto-revert--timeout)
+             buf "C" (auto-revert--timeout))
             (should (equal (buffer-string) "C"))))
 
       ;; Clean up.
       (ignore-errors (kill-buffer buf))
       (ignore-errors (delete-file file-1))
-      (ignore-errors (delete-file file-2)))))
+      (ignore-errors (delete-file file-2))))))
 
 (auto-revert--deftest-remote auto-revert-test06-write-file
   "Test `write-file' in `auto-revert-mode' for remote buffers.")
-- 
2.28.0

Re: master 262d0c6: Mark some tests as expensive

[Michael Albinus]

Stefan Kangas <stefan@marxist.se> writes:

> Hi Michael,

Hi Stefan,

>> Would be helpful. However, there have been discussions (and changes) of
>> the timeouts in the past, so I'm not sure we can reduce them w/o causing
>> damage. Please check the git history of autorevert-tests.el for such
>> changes, hopefully there are bug numbers in the git log you could consult
>> for the respective discussions. Otherwise, a search on the emacs-devel
>> and emacs-bugs MLs for the given timeframe of such a change might be helpful.
>
> So I have attached here a patch that employs the above mentioned
> techniques somewhat aggressively to reduce the running time as follows:
>
> Ran 7 tests, 7 results as expected, 0 unexpected (2020-10-18
> 19:38:29+0200, 2.323729 sec)

Thanks!

> Everything works fine here, but I don't know if any of this would cause
> any problems elsewhere.  One idea is to just push it and see what
> breaks, and then adapt accordingly.
>
> Please let me know what you think.

Yes, let's go this way. Maybe there will be problems in the remote case,
but this we could adapt later on.

> Also, I've had some trouble finding the past discussions about this.
>
> But I'm not sure if any of that is even relevant here.  Let me know if
> you have any ideas for what I could look for.

No, my history isn't organized as good as it should :-(

> * test/lisp/autorevert-tests.el (auto-revert--timeout): Make into
> defun and shorten timeout by a factor 10.

Why is it a defun now? Likely, I have overseen the reason ...

Best regards, Michael.

> Best regards,
> Stefan Kangas

Re: master 262d0c6: Mark some tests as expensive

[Stefan Kangas]

Michael Albinus <michael.albinus@gmx.de> writes:

>> Everything works fine here, but I don't know if any of this would cause
>> any problems elsewhere.  One idea is to just push it and see what
>> breaks, and then adapt accordingly.
>>
>> Please let me know what you think.
>
> Yes, let's go this way. Maybe there will be problems in the remote case,
> but this we could adapt later on.

OK!  I've re-indented the code (due to the new macro) and pushed it to
master as commit 1a32809d2b.

>> * test/lisp/autorevert-tests.el (auto-revert--timeout): Make into
>> defun and shorten timeout by a factor 10.
>
> Why is it a defun now? Likely, I have overseen the reason ...

It's just to have a different value when auto-revert-interval is
let-bound (as in the `with-auto-revert-test' macro).  But we don't use
this macro in all the tests.

Re: master 262d0c6: Mark some tests as expensive

[Michael Albinus]

Stefan Kangas <stefan@marxist.se> writes:

Hi Stefan,

>> Yes, let's go this way. Maybe there will be problems in the remote case,
>> but this we could adapt later on.
>
> OK!  I've re-indented the code (due to the new macro) and pushed it to
> master as commit 1a32809d2b.

Thanks. As expected, the remote tests do not pass. But for other reasons
but the changed timing ...

Best regards, Michael.