This is an archive of the discontinued Mercurial Phabricator instance.

tests: stabilize test-rename-merge2.t on Windows
ClosedPublic

Authored by mharbison72 on Feb 24 2020, 5:47 PM.

Details

Summary

I have no idea why, but this shifted in b4057d001760.

Diff Detail

Repository
rHG Mercurial
Branch
default
Lint
No Linters Available
Unit
No Unit Test Coverage

Event Timeline

mharbison72 created this revision.Feb 24 2020, 5:47 PM
marmoute accepted this revision.Feb 24 2020, 6:37 PM
marmoute added a subscriber: marmoute.

Since the line does not occurs on linux, this probably got affected by some other change without being noticed.

Since the line does not occurs on linux, this probably got affected by some other change without being noticed.

Makes sense for why it was missed, but I bisected it back to the referenced commit. I know @martinvonz made some other related changes prior to that commit that had similar test fallout on Windows, but it isn't obvious to me why the referenced commit would change the order of when the background thread spins up.

Since run-test has no idea where to put (?) line with the surounding change, they often end up preserved at start/end of the changeset section. This is probably just something like that.

Since the line does not occurs on linux, this probably got affected by some other change without being noticed.

Makes sense for why it was missed, but I bisected it back to the referenced commit. I know @martinvonz made some other related changes prior to that commit that had similar test fallout on Windows, but it isn't obvious to me why the referenced commit would change the order of when the background thread spins up.

Perhaps the line doesn't actually happen before that commit? You can try removing the (?) in the commit before my commit and see if that line is actually printed. I'm thinking that that commit pushed the number of files to close above some threshold that made it get printed. In that case, it was probably some of my earlier commits that made it no longer get printed and we just didn't notice since (?) means optional (no error if it doesn't get printed).

Since the line does not occurs on linux, this probably got affected by some other change without being noticed.

Makes sense for why it was missed, but I bisected it back to the referenced commit. I know @martinvonz made some other related changes prior to that commit that had similar test fallout on Windows, but it isn't obvious to me why the referenced commit would change the order of when the background thread spins up.

Perhaps the line doesn't actually happen before that commit? You can try removing the (?) in the commit before my commit and see if that line is actually printed. I'm thinking that that commit pushed the number of files to close above some threshold that made it get printed. In that case, it was probably some of my earlier commits that made it no longer get printed and we just didn't notice since (?) means optional (no error if it doesn't get printed).

I can confirm that it was required in e1ecfc7c84be, and reconfirmed that it is needed in the latest code by removing the line from both. I'm not too worried about why it popped up- it just seemed odd that it didn't with the earlier followup patch I added to fix similar conditionalizing of this exact text elsewhere, and the commit where it started failing looked innocuous.

Since the line does not occurs on linux, this probably got affected by some other change without being noticed.

Makes sense for why it was missed, but I bisected it back to the referenced commit. I know @martinvonz made some other related changes prior to that commit that had similar test fallout on Windows, but it isn't obvious to me why the referenced commit would change the order of when the background thread spins up.

Perhaps the line doesn't actually happen before that commit? You can try removing the (?) in the commit before my commit and see if that line is actually printed. I'm thinking that that commit pushed the number of files to close above some threshold that made it get printed. In that case, it was probably some of my earlier commits that made it no longer get printed and we just didn't notice since (?) means optional (no error if it doesn't get printed).

I can confirm that it was required in e1ecfc7c84be, and reconfirmed that it is needed in the latest code by removing the line from both. I'm not too worried about why it popped up- it just seemed odd that it didn't with the earlier followup patch I added to fix similar conditionalizing of this exact text elsewhere, and the commit where it started failing looked innocuous.

Yeah, I'm not worried about it either. Thanks for checking. (I wasn't holding off queuing this one because of that, I was/am just busy with other stuff.)

pulkit accepted this revision.Feb 25 2020, 7:22 AM
This revision is now accepted and ready to land.Feb 25 2020, 7:22 AM
This revision was automatically updated to reflect the committed changes.