[PATCH] run-tests: paths do automatically match on Windows
mads at kiilerich.com
Thu Oct 11 06:41:06 CDT 2012
On 10/05/2012 06:00 AM, Simon Heimberg wrote:
> # HG changeset patch
> # User Simon Heimberg <simohe at besonet.ch>
> # Date 1349409552 -7200
> # Node ID 2bb109db2f30bf6efc7999dc53c7a87c4d612b41
> # Parent 5e641a809b056d42c495dbd69ca991ddb13c3c5d
> run-tests: paths do automatically match on Windows
For reference: This is an alternative to 9910f60a37ee .
The advantage of the current '(glob)' approach is that it requires
explicit markup ... and that is also the main disadvantage.
The real problem here is IMO that ui.slash doesn't control all the
places where we display native paths ... and there is no clean simple
way to achieve that. We just want to handle that in the test suite for now.
Most of the tests do now have the necessary markup and the maintenance
overhead of the current approach is relatively low.
as an exception. (That kind of need for markup could however also be
caught by a check-code warning.)
It would perhaps have been better to use the proposed approach from the
beginning, but I don't think there is any real benefit from switching now.
Anyway, some comments:
> This makes writing tests easier. And it could speed up the test suite a little
> bit because more lines will match without a glob comparison.
> When a path MUST be written with / on any os, the (esc) syntax can be used.
But nobody will ever bother enough to use that markup for that purpose
> also update check-code accordingly and warn about glob with no glob (? or *)
"also" -> separate patch.
> diff -r 5e641a809b05 -r 2bb109db2f30 contrib/check-code.py
> --- a/contrib/check-code.py Fre Okt 05 05:35:02 2012 +0200
> +++ b/contrib/check-code.py Fre Okt 05 05:59:12 2012 +0200
> @@ -100,11 +100,12 @@
> "explicit exit code checks unnecessary"),
> (uprefix + r'set -e', "don't use set -e"),
> (uprefix + r'\s', "don't indent commands, use > for continued lines"),
> - (r'^ saved backup bundle to \$TESTTMP.*\.hg$',
> - "use (glob) to match Windows paths too"),
> # warnings
> - 
> + [
> + (r'^ [^*?\n]* \(glob\)$',
> + "warning: remove (glob), Windows paths match automatically"),
> + ]
There is no need for a new warning. Just clean it up once and for all
(preferably in a separate patch).
> diff -r 5e641a809b05 -r 2bb109db2f30 tests/run-tests.py
> --- a/tests/run-tests.py Fre Okt 05 05:35:02 2012 +0200
> +++ b/tests/run-tests.py Fre Okt 05 05:59:12 2012 +0200
> @@ -536,6 +536,13 @@
> res += re.escape(c)
> return rematch(res, l)
> +if os.name == 'nt':
> + def pathmatch(el, l):
> + return len(el) != len(l) and el == l.replace('\\', '/')
Why the '!=' ? It seems completely wrong.
And it seems like an optimization ... but will it really give any
optimization that is worth it? s.replace() is "fast" - especially if
I am sure there are other and bigger bottlenecks in run-tests.
> + def pathmatch(el, l):
> + return False
> def linematch(el, l):
> if el == l: # perfect match (fast)
> return True
> @@ -545,7 +552,8 @@
> el.endswith(" (esc)\n") and
> (el[:-7].decode('string-escape') + '\n' == l or
> el[:-7].decode('string-escape').replace('\r', '') +
> - '\n' == l and os.name == 'nt'))):
> + '\n' == l and os.name == 'nt') or
> + pathmatch(el, l))):
That is two completely different approaches to handling 'nt' specific
functionality on two subsequent lines. Please be consistent.
The new indentation is also not consistent with the existing.
It would perhaps be better to move more to a platform dependent match
function, but having the test explicit inline is perhaps more readable.
The name "patchmatch" is perhaps also not descriptive. "slashmatch"
would perhaps be better.
More information about the Mercurial-devel