Patchwork [3,of,3] test-progress: disable mocking-time tests on chg

login
register
mail settings
Submitter Yuya Nishihara
Date April 8, 2016, 3:23 p.m.
Message ID <758f31c11f8b9d400e7b.1460129039@mimosa>
Download mbox | patch
Permalink /patch/14432/
State Accepted
Headers show

Comments

Yuya Nishihara - April 8, 2016, 3:23 p.m.
# HG changeset patch
# User Yuya Nishihara <yuya@tcha.org>
# Date 1458517796 25200
#      Sun Mar 20 16:49:56 2016 -0700
# Node ID 758f31c11f8b9d400e7b1e0e7f35d3315fb12186
# Parent  369d151ba12b38708c9e8094c157a56bb52e2d1f
test-progress: disable mocking-time tests on chg

It's hard to make these tests compatible with chg because a mocked time.time()
is recorded and accessed by progbar at random timing. I don't think it's worth
fixing this test as it is considered a unit test of time estimates, so just
ignores on chg.
timeless - April 8, 2016, 3:46 p.m.
Yuya Nishihara wrote:
> ignores on chg.

ignores => ignore | ignore it

otherwise, the change itself seems reasonable.
Jun Wu - April 8, 2016, 4:18 p.m.
On 04/08/2016 04:23 PM, Yuya Nishihara wrote:
> test-progress: disable mocking-time tests on chg

The series look good to me.

I think test-progress.t would be fixable by adding MOCKTIME to sensitive
environment variable. Or, just change it to HGMOCKTIME since that would
be sensitive to chg. But just skipping them also makes sense.

However the API to support change sensitive part of confighash won't arrive
soon. Originally, I would like to do that in chgserver.py and then with
devel-warn mentioned by marmoute, it seems the logic to mark which config
is sensitive is better living in ui.py, config.py instead of chgserver.py.

But I'm still not sure where to put the API to change sensitive config
or environment variables. chgserver.py actually makes a lot of sense if
it is considered "core" because it's the only one requiring the stuff.
Moving it to files like extensions.py, ui.py or config.py looks over-
design by me, because I don't think there are future users.

If "import chgserver" in extensions.py for devel-warn is acceptable,
I would be happy and just go ahead with adding the sensitive APIs to
chgserver.py and devel-warn, like http://sf.p.lihdd.net/hg/ff7650bef711.txt
Yuya Nishihara - April 9, 2016, 8:01 a.m.
On Fri, 8 Apr 2016 17:18:56 +0100, Jun Wu wrote:
> On 04/08/2016 04:23 PM, Yuya Nishihara wrote:
> > test-progress: disable mocking-time tests on chg  
> 
> The series look good to me.
> 
> I think test-progress.t would be fixable by adding MOCKTIME to sensitive
> environment variable. Or, just change it to HGMOCKTIME since that would
> be sensitive to chg. But just skipping them also makes sense.

I've tried HGMOCKTIME, but the problem seemed not that simple, and I stopped
investigation as it wouldn't be worth making this test pass on chg.

> However the API to support change sensitive part of confighash won't arrive
> soon. Originally, I would like to do that in chgserver.py and then with
> devel-warn mentioned by marmoute, it seems the logic to mark which config
> is sensitive is better living in ui.py, config.py instead of chgserver.py.
> 
> But I'm still not sure where to put the API to change sensitive config
> or environment variables. chgserver.py actually makes a lot of sense if
> it is considered "core" because it's the only one requiring the stuff.
> Moving it to files like extensions.py, ui.py or config.py looks over-
> design by me, because I don't think there are future users.
> 
> If "import chgserver" in extensions.py for devel-warn is acceptable,
> I would be happy and just go ahead with adding the sensitive APIs to
> chgserver.py and devel-warn, like http://sf.p.lihdd.net/hg/ff7650bef711.txt

I don't follow the context behind this patch, but some of the problems
detected by _chgcompatcheck would happen on plain command server. That might
justify moving chgserver._configsections somewhere else.

However, the patch seems overly complicated compared to the benefit provided
by it.
Jun Wu - April 9, 2016, 8:32 p.m.
On 04/09/2016 09:01 AM, Yuya Nishihara wrote:
> I've tried HGMOCKTIME, but the problem seemed not that simple, and I
> stopped investigation as it wouldn't be worth making this test pass on chg.

Yep. I had tried that (before the sprint) as well. It seems it solves some
problem but not all. Therefore I'm okay with just skipping them.
(This is not important anyway. I want to discuss the chg checker script below)

> I don't follow the context behind this patch, but some of the problems
> detected by _chgcompatcheck would happen on plain command server. That
> might justify moving chgserver._configsections somewhere else.
>
> However, the patch seems overly complicated compared to the benefit
> provided by it.

My V1 patch is a standalone check-chg-compat script:
http://patchwork.serpentine.com/patch/14230/

marmoute has rejected that one telling me I should use devel-warn on IRC.
He marked the patch as "Change Requested" without replying the mail though.
I somehow agree that we may want devel-warn eventually but it is hard to
do now because:

1. devel-warn is enabled for all tests, which means to enable it, we have
to fix all problematic before the devel-warn patch. To fix all extensions,
we have to get the whitelist API refactoring.
2. devel-warn in extensions.loadall() does not work well with chg because they
are only printed once if all extensions are loaded without issues.
3. It cannot be used directly to test third party extensions without side
effect of an hg command.
4. It still looks a bit weird hacking os.environ etc.

Therefore, I think a standalone checker script is the way to go for now because:

1. It allows us to fix problematic extensions one by one instead of fixing
them all the once.
2. It does not pollute core files like "extensions.py".
3. It's easy to reuse the script to check third party extension projects,
just run something like "check-chg-compat.py fb-hgext/*.py".

What do you think?
Yuya Nishihara - April 10, 2016, 10:57 a.m.
On Sat, 9 Apr 2016 21:32:51 +0100, Jun Wu wrote:
> On 04/09/2016 09:01 AM, Yuya Nishihara wrote:
> > I don't follow the context behind this patch, but some of the problems
> > detected by _chgcompatcheck would happen on plain command server. That
> > might justify moving chgserver._configsections somewhere else.
> >
> > However, the patch seems overly complicated compared to the benefit
> > provided by it.  
> 
> My V1 patch is a standalone check-chg-compat script:
> http://patchwork.serpentine.com/patch/14230/
> 
> marmoute has rejected that one telling me I should use devel-warn on IRC.
> He marked the patch as "Change Requested" without replying the mail though.
> I somehow agree that we may want devel-warn eventually but it is hard to
> do now because:
> 
> 1. devel-warn is enabled for all tests, which means to enable it, we have
> to fix all problematic before the devel-warn patch. To fix all extensions,
> we have to get the whitelist API refactoring.
> 2. devel-warn in extensions.loadall() does not work well with chg because they
> are only printed once if all extensions are loaded without issues.
> 3. It cannot be used directly to test third party extensions without side
> effect of an hg command.
> 4. It still looks a bit weird hacking os.environ etc.
> 
> Therefore, I think a standalone checker script is the way to go for now because:
> 
> 1. It allows us to fix problematic extensions one by one instead of fixing
> them all the once.
> 2. It does not pollute core files like "extensions.py".
> 3. It's easy to reuse the script to check third party extension projects,
> just run something like "check-chg-compat.py fb-hgext/*.py".
> 
> What do you think?

I tend to agree with marmoute in that devel-warn would be nicer, but that
would be true only if the implementation were clean. As far as I can guess
from your new patch, it would be hard to avoid dirty hacks such as monkey-
patching core functions.

So, I think a standalone script is good start.
Jun Wu - April 10, 2016, 12:35 p.m.
On 04/10/2016 11:57 AM, Yuya Nishihara wrote:
> I tend to agree with marmoute in that devel-warn would be nicer, but that
> would be true only if the implementation were clean. As far as I can guess
> from your new patch, it would be hard to avoid dirty hacks such as monkey-
> patching core functions.
>
> So, I think a standalone script is good start.

I agree with you about all your points about devel-warn.

So could we get the checker script (patchwork.serpentine.com/patch/14230/) in?
Or, do I need to resend it? I think it would be helpful to stop new code
breaking chg somehow.
Yuya Nishihara - April 10, 2016, 1:54 p.m.
On Sun, 10 Apr 2016 13:35:46 +0100, Jun Wu wrote:
> On 04/10/2016 11:57 AM, Yuya Nishihara wrote:
> > I tend to agree with marmoute in that devel-warn would be nicer, but that
> > would be true only if the implementation were clean. As far as I can guess
> > from your new patch, it would be hard to avoid dirty hacks such as monkey-
> > patching core functions.
> >
> > So, I think a standalone script is good start.  
> 
> I agree with you about all your points about devel-warn.
> 
> So could we get the checker script (patchwork.serpentine.com/patch/14230/) in?
> Or, do I need to resend it? I think it would be helpful to stop new code
> breaking chg somehow.

Please resend.

You might be interested in the ast module to investigate the whole execution
paths, but I don't know how hard it would be.
Jun Wu - April 10, 2016, 2:26 p.m.
On 04/10/2016 02:54 PM, Yuya Nishihara wrote:
> You might be interested in the ast module to investigate the whole execution
> paths, but I don't know how hard it would be.

I thought about that but it's very hard to do it correctly since you are
essentially implementing the python language. Just consider calling another
method from uisetup and that method accesses os.environ.
Pierre-Yves David - April 13, 2016, 6:42 a.m.
On 04/09/2016 01:32 PM, Jun Wu wrote:
> On 04/09/2016 09:01 AM, Yuya Nishihara wrote:
>> I've tried HGMOCKTIME, but the problem seemed not that simple, and I
>> stopped investigation as it wouldn't be worth making this test pass 
>> on chg.
>
> Yep. I had tried that (before the sprint) as well. It seems it solves 
> some
> problem but not all. Therefore I'm okay with just skipping them.
> (This is not important anyway. I want to discuss the chg checker 
> script below)
>
>> I don't follow the context behind this patch, but some of the problems
>> detected by _chgcompatcheck would happen on plain command server. That
>> might justify moving chgserver._configsections somewhere else.
>>
>> However, the patch seems overly complicated compared to the benefit
>> provided by it.
>
> My V1 patch is a standalone check-chg-compat script:
> http://patchwork.serpentine.com/patch/14230/
>
> marmoute has rejected that one telling me I should use devel-warn on IRC.
> He marked the patch as "Change Requested" without replying the mail 
> though.
> I somehow agree that we may want devel-warn eventually but it is hard to
> do now because:
>
> 1. devel-warn is enabled for all tests, which means to enable it, we have
> to fix all problematic before the devel-warn patch. To fix all 
> extensions,
> we have to get the whitelist API refactoring.

Actually, not entirely. If fixing all extensions immediatly is too much 
a trouble we could have the devel warning disabled, either by default or 
for specific tests. Fixing them iteratively after than.

This would avoid implementing the feature twice, once in the script, and 
once for the markers. It also increase the chance that we actually end 
up with a devel warning.
Pierre-Yves David - April 13, 2016, 6:56 a.m.
On 04/08/2016 08:23 AM, Yuya Nishihara wrote:
> # HG changeset patch
> # User Yuya Nishihara <yuya@tcha.org>
> # Date 1458517796 25200
> #      Sun Mar 20 16:49:56 2016 -0700
> # Node ID 758f31c11f8b9d400e7b1e0e7f35d3315fb12186
> # Parent  369d151ba12b38708c9e8094c157a56bb52e2d1f
> test-progress: disable mocking-time tests on chg
>
> It's hard to make these tests compatible with chg because a mocked time.time()
> is recorded and accessed by progbar at random timing. I don't think it's worth
> fixing this test as it is considered a unit test of time estimates, so just
> ignores on chg.

For some reason, this patch 3, report the whole test run for 
test-progress.t as skipped. regardless of any other error.

Can you investigate.
Pierre-Yves David - April 13, 2016, 7:02 a.m.
On 04/12/2016 11:56 PM, Pierre-Yves David wrote:
>
>
> On 04/08/2016 08:23 AM, Yuya Nishihara wrote:
>> # HG changeset patch
>> # User Yuya Nishihara <yuya@tcha.org>
>> # Date 1458517796 25200
>> #      Sun Mar 20 16:49:56 2016 -0700
>> # Node ID 758f31c11f8b9d400e7b1e0e7f35d3315fb12186
>> # Parent  369d151ba12b38708c9e8094c157a56bb52e2d1f
>> test-progress: disable mocking-time tests on chg
>>
>> It's hard to make these tests compatible with chg because a mocked 
>> time.time()
>> is recorded and accessed by progbar at random timing. I don't think 
>> it's worth
>> fixing this test as it is considered a unit test of time estimates, 
>> so just
>> ignores on chg.
>
> For some reason, this patch 3, report the whole test run for 
> test-progress.t as skipped. regardless of any other error.
>
> Can you investigate.

Top tier investigation reveal that I forgot to pick patch 1…

Things works fine with it. Patch 1 and 3 are pushed.
Jun Wu - April 13, 2016, 10:41 a.m.
On 04/13/2016 07:42 AM, Pierre-Yves David wrote:
> Actually, not entirely. If fixing all extensions immediatly is too much a
> trouble we could have the devel warning disabled, either by default or for
> specific tests. Fixing them iteratively after than.

How about other issues?

1. Hacking stdlib is ugly
2. chg will only display devel-warn in extensions.loadall once (i.e. a second
run won't display these warnings)
3. Implementing the devel-warn itself require large refactoring if "import
chgserver" in extensions.py is not allowed.


> This would avoid implementing the feature twice, once in the script, and
> once for the markers. It also increase the chance that we actually end up
> with a devel warning.

There are too many tests related so only disabling by default makes sense.
If it is disabled by default then we lose the meaning of having it: prevent
new code from breaking chg.
Pierre-Yves David - April 14, 2016, 7:23 p.m.
On 04/13/2016 03:41 AM, Jun Wu wrote:
> On 04/13/2016 07:42 AM, Pierre-Yves David wrote:
>> Actually, not entirely. If fixing all extensions immediatly is too 
>> much a
>> trouble we could have the devel warning disabled, either by default 
>> or for
>> specific tests. Fixing them iteratively after than.
>
> How about other issues?
>
> 1. Hacking stdlib is ugly

Yes it is. But we don't have a better option, do we? Given that the 
monkey patch will be temporary and only in the case where devel-warning 
are enabled, I think this is okay. We do some awful stuff in core from 
time to time when we don't have better option.

> 2. chg will only display devel-warn in extensions.loadall once (i.e. a 
> second
> run won't display these warnings)

This is fine, it will be caught by the non-chg runs. Especially the 
tests are run with the devel-warning on and I expect most people to see 
them there.

> 3. Implementing the devel-warn itself require large refactoring if 
> "import
> chgserver" in extensions.py is not allowed.

Can you elaborate about this refactoring? From my understanding, the 
list of "watched" config and env were to be moved into extensions and 
that is it. Anything else is required?

>
>
>> This would avoid implementing the feature twice, once in the script, and
>> once for the markers. It also increase the chance that we actually 
>> end up
>> with a devel warning.
>
> There are too many tests related so only disabling by default makes 
> sense.
> If it is disabled by default then we lose the meaning of having it: 
> prevent
> new code from breaking chg.

How many test actually breaks? How many core extensions are currently 
broken?

Having a devel warning will allow us to slowly fixing all the issues, 
when they will all be fixed (or blacklisted) we can just get the benefit 
by flipping a small switch.

Cheers,
Sean Farley - April 14, 2016, 8:34 p.m.
Pierre-Yves David <pierre-yves.david@ens-lyon.org> writes:

> On 04/13/2016 03:41 AM, Jun Wu wrote:
>> On 04/13/2016 07:42 AM, Pierre-Yves David wrote:
>>> Actually, not entirely. If fixing all extensions immediatly is too 
>>> much a
>>> trouble we could have the devel warning disabled, either by default 
>>> or for
>>> specific tests. Fixing them iteratively after than.
>>
>> How about other issues?
>>
>> 1. Hacking stdlib is ugly
>
> Yes it is. But we don't have a better option, do we? Given that the 
> monkey patch will be temporary and only in the case where devel-warning 
> are enabled, I think this is okay. We do some awful stuff in core from 
> time to time when we don't have better option.

Why can't we have an abstraction in pycompat? e.g.

pycompat.hgiteritems()
Yuya Nishihara - April 15, 2016, 12:54 p.m.
On Thu, 14 Apr 2016 12:23:08 -0700, Pierre-Yves David wrote:
> On 04/13/2016 03:41 AM, Jun Wu wrote:
> > 1. Hacking stdlib is ugly  
> 
> Yes it is. But we don't have a better option, do we? Given that the 
> monkey patch will be temporary and only in the case where devel-warning 
> are enabled, I think this is okay.

My concern is monkey-patched functions could last forever if an extension
makes aliases to them in uisetup() or extsetup(). That would be silly, though.

And I'm not quite happy with adding unloved hacks to the extensions module
only for getting niche devel-warnings.
Jun Wu - April 15, 2016, 4:05 p.m.
On 04/14/2016 08:23 PM, Pierre-Yves David wrote:
> Yes it is. But we don't have a better option, do we? Given that the monkey
> patch will be temporary and only in the case where devel-warning are
> enabled, I think this is okay. We do some awful stuff in core from time to
> time when we don't have better option.

I don't have strong opinion on this. But a standalone check script is a
(temporary) option.

>> 2. chg will only display devel-warn in extensions.loadall once (i.e. a
>> second run won't display these warnings)
>
> This is fine, it will be caught by the non-chg runs. Especially the tests
> are run with the devel-warning on and I expect most people to see them
> there.

Then we may have to manually add "#if chg" in tests, which is not cool.

> Can you elaborate about this refactoring? From my understanding, the list of
> "watched" config and env were to be moved into extensions and that is it.
> Anything else is required?

I used to agree that moving the whitelist logic to extensions.py is a solution.

But later I felt it was not ideal:

- extensions should not have heavy "config" related logic
- the whitelist logic's best place is "config" or "ui": something like
ui.marksenstive.

Even fancier, we can have something like an "extras" dict attached to every
config items and currently it would look like {'source': '~/.hgrc', 'senstive':
true}. I then associated this with my bigger ui/config refactoring plan and
it instantly felt much bigger. Besides, the "extras" idea looks nice but
practically it does not work because:

- extensions may want to mark config items with glob patterns
- we also have environment variables to be marks

Besides, I felt that chgserver's final position is in "mercurial/" just like
"commandserver.py" so it actually makes sense to implement the whitelist API
there, which is even cleaner then hacking ui or config or extensions imo.

Therefore I just felt unclear about the most clean way to go and postpone all 
these before we have a good-looking-to-everyone solution.

> How many test actually breaks? How many core extensions are currently
> broken?

At least tens of tests IIRC. I don't really want to deal with them manually.
5 extensions, according to http://patchwork.serpentine.com/patch/14231/

> Having a devel warning will allow us to slowly fixing all the issues, when
> they will all be fixed (or blacklisted) we can just get the benefit by
> flipping a small switch.

I still think a separate script is a practically better solution now because:

- I can skip dealing with at least tens of tests.
- It does make the core code worse so people are happy.

My suggestion is to have a temporary checker now and it will stay until
all extensions are fixed, at which time we can consider move the logic to core
as devel-warn.
Yuya Nishihara - April 15, 2016, 4:51 p.m.
On Fri, 15 Apr 2016 17:05:54 +0100, Jun Wu wrote:
> Besides, I felt that chgserver's final position is in "mercurial/" just like
> "commandserver.py" so it actually makes sense to implement the whitelist API
> there, which is even cleaner then hacking ui or config or extensions imo.

Perhaps. I didn't do that at the beginning of the porting because I had to
promote commandserver._servicemap to somewhere upper layer in order to avoid
import cycle.

Patch

diff --git a/tests/test-progress.t b/tests/test-progress.t
--- a/tests/test-progress.t
+++ b/tests/test-progress.t
@@ -182,6 +182,8 @@  test immediate progress completion
 
 test delay time estimates
 
+#if no-chg
+
   $ cat > mocktime.py <<EOF
   > import os
   > import time
@@ -250,6 +252,8 @@  Time estimates should not fail when ther
   loop [  <=>                                             ] 3\r (no-eol) (esc)
                                                               \r (no-eol) (esc)
 
+#endif
+
 test line trimming by '[progress] width', when progress topic contains
 multi-byte characters, of which length of byte sequence and columns in
 display are different from each other.