1394391 - vcs.py does not seem to handle listing changes to hg repositories correctly

Status: RESOLVED FIXED

(Testing :: General, defect)

Description

[John Ford [:jhford] CET/CEST Berlin Time]

It looks like the HgHelper files_changed method is not splitting the output into newlines and as such is treating the listing of files as a string instead of a list of strings.

https://dxr.mozilla.org/mozilla-central/source/tools/tryselect/vcs.py#149

Comment 1

[Andrew Halberstadt [:ahal]]

Attachment: Bug 1394391 - [tryselect] Split hg implementation of files_changed to a list,

This fixes a regression from bug 1384593. I was going to add a test for this, but it would be
a better use of time to fix bug 1185599 and add a test there. We already have vcs tests in other
parts of the tree so consolidating them into a single module is likely easier than standing up
another one-off vcs tester.

Review commit: https://reviewboard.mozilla.org/r/173238/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/173238/

Comment 2

[John Ford [:jhford] CET/CEST Berlin Time]

Comment on attachment 8901800 [details]
Bug 1394391 - [tryselect] Split hg implementation of files_changed to a list,

https://reviewboard.mozilla.org/r/173236/#review178598

Looks good to me!  I did try to run ./mach try -p linux with this applied manually, but I get the following error:

Specified path "/Users/jhford/taskcluster/mozilla-central/tools/tryselect/test/**" is not a directory under the srcdir, unable to specify tests outside of the srcdir

Not sure if that's related to this change or to me applying this patch manually without merging in your branch.

Comment 3

[John Ford [:jhford] CET/CEST Berlin Time]

This still happens if I commit the change locally, so I suspect that there's some path calculations going wrong here.  Should that be a new bug?

Comment 4

[Andrew Halberstadt [:ahal]]

I'm seeing this too. I'm not sure if this second failure is also a regression, if it's always been broken, or if the message is just misleading.

I looked into this a little, and this code path is only hit when *not* passing in -u, paths or tags (e.g ./mach try -b o -p linux) and it attempts to find default tests to run. I'm almost tempted to just remove this whole thing entirely because it's assuming people *want* to run tests in the first place.

If you're just trying to run builds, you might be able to work around this issue by running ./mach try -b o -p linux -u none

I'm happy re-using this bug or filing a new one either way.

Comment 5

[Andrew Halberstadt [:ahal]]

Digging into this, I'm fairly sure this has been broken for a long time and no one has noticed/said anything as they use '-u none' when wanting to only submit builds. So I'd recommend landing this fix for now and filing a follow-up. At the very least there will be some discussion on how to fix it. (Though to be honest I'm unlikely to be the one to volunteer as A) I already consider try syntax to be unofficially deprecated, and B) I have no idea what that code is trying to accomplish).

I don't think your specific patch series has anything to do with this. Try pushing with something like './mach try -b do -p linux -u none' (using your desired -b/-p). Or better yet, use './mach try fuzzy'

Comment 6

[John Ford [:jhford] CET/CEST Berlin Time]

sounds good to me!

Comment 7

[John Ford [:jhford] CET/CEST Berlin Time]

Comment on attachment 8901800 [details]
Bug 1394391 - [tryselect] Split hg implementation of files_changed to a list,

https://reviewboard.mozilla.org/r/173238/#review179038

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3942e9db3da3
[tryselect] Split hg implementation of files_changed to a list, r=jhford

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/3942e9db3da3