Page MenuHomePhabricator

tests: drop a few unnecessary "(glob)"
AbandonedPublic

Authored by martinvonz on Feb 18 2019, 12:24 AM.

Details

Reviewers
mharbison72
Group Reviewers
hg-reviewers
Summary

I believe thise are unnecessary since a8d3a4be066e (windows: use
util.localpath for repo-relative paths in getuipathfn(), 2019-02-11),
but someone with a Windows machine should confirm.

Diff Detail

Repository
rHG Mercurial
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

martinvonz created this revision.Feb 18 2019, 12:24 AM

The tests run clean with this.

But I'm a bit confused. The test harness has been doing '\' -> '/' conversion without (glob) now for a little over a year, and it complains if there's a trailing (glob) and no '\' -> '/' conversion. That's not been happening here, and made me suspicious. These globs predate that functionality slightly, so I'm not sure the meaning of the referenced commit (which seems to say nothing will change because ui.slash is set).

To be clear, these paths are still being printed with '\', and the test harness is accommodating that. But I'm not sure if that's what you intended.

The tests run clean with this.
But I'm a bit confused. The test harness has been doing '\' -> '/' conversion without (glob) now for a little over a year, and it complains if there's a trailing (glob) and no '\' -> '/' conversion. That's not been happening here, and made me suspicious. These globs predate that functionality slightly, so I'm not sure the meaning of the referenced commit (which seems to say nothing will change because ui.slash is set).

You're right, and even if I go back to bdcaf612e75a (where you added these globs), it seems like it should have been converted to slashes already there (because the test runner set ui.slash back then, too, and the code seemed to respect that). Do you have time to go back and see if the globbing was never needed?

Maybe the "no complaining from test harness" is because these lines contain a "?", which is a glob character, so maybe it doesn't warn then?

To be clear, these paths are still being printed with '\', and the test harness is accommodating that. But I'm not sure if that's what you intended.

Yes, I hope that my recent changes has made it so more commands, not fewer, print paths in the native form.

The tests run clean with this.
But I'm a bit confused. The test harness has been doing '\' -> '/' conversion without (glob) now for a little over a year, and it complains if there's a trailing (glob) and no '\' -> '/' conversion. That's not been happening here, and made me suspicious. These globs predate that functionality slightly, so I'm not sure the meaning of the referenced commit (which seems to say nothing will change because ui.slash is set).

You're right, and even if I go back to bdcaf612e75a (where you added these globs), it seems like it should have been converted to slashes already there (because the test runner set ui.slash back then, too, and the code seemed to respect that). Do you have time to go back and see if the globbing was never needed?

It was needed. In fact the test runner itself added it on Windows. (There's logic in there to add it to the output if changing '\' to '/' results in a match.) I bisected the HGPLAIN=1 hg status --cwd a test back to 7e3b145f8247, where the trail went cold- it fails there too where it was added. Backing up one more and reverting the test to that also fails, so it wasn't a code change there causing it.

(As an aside, it would be awesome if bisect accepted a --force to ignore the dirty working copy check. Then I could take out the globs, and see where it started working without having to manually update to each candidate. I guess the question then is whether to merge the dirty changes on each update, or just revert to the dirty changes as-is. I guess I'd lean towards the latter for automated bisection.)

Maybe the "no complaining from test harness" is because these lines contain a "?", which is a glob character, so maybe it doesn't warn then?

Strangely, no. I added a (glob) to a random status test that listed only the file with an unknown status in 37a0ad669051 and 37b33c34bf4f, and they both took away the stray (glob).

To be clear, these paths are still being printed with '\', and the test harness is accommodating that. But I'm not sure if that's what you intended.

Yes, I hope that my recent changes has made it so more commands, not fewer, print paths in the native form.

I had forgotten about this patch and it's not very important anyway. I don't know how I'm supposed to fix it, so I'll just drop. @mharbison72, let me know if there was something else I should have done.

martinvonz abandoned this revision.May 9 2019, 1:03 PM

I had forgotten about this patch and it's not very important anyway. I don't know how I'm supposed to fix it, so I'll just drop. @mharbison72, let me know if there was something else I should have done.

Test stability-wise, we should be OK with or without it. I'm not sure of the intention with the various config options, and Yuya's concern about not changing the output in the thread below.

What's happening now is the commands output paths with '\', and the test harness automatically converts to '/' without needing an explicit (glob). I raised these diffs as an issue at one point because it was outputting with '/', and the test harness complains about those paths having (glob) on Windows. It looks like this is basically the same as https://www.mercurial-scm.org/pipermail/mercurial-devel/2019-February/127959.html