Skip to content
This repository was archived by the owner on Jul 28, 2024. It is now read-only.

Fix failing VersionPress\Tests\Workflow\CloneMergeTest::dateModifiedMergesAutomatically test#1376

Closed
candrews wants to merge 1 commit intoversionpress:masterfrom
candrews:fix-dateModifiedMergesAutomatically
Closed

Fix failing VersionPress\Tests\Workflow\CloneMergeTest::dateModifiedMergesAutomatically test#1376
candrews wants to merge 1 commit intoversionpress:masterfrom
candrews:fix-dateModifiedMergesAutomatically

Conversation

@candrews
Copy link
Copy Markdown
Contributor

@candrews candrews commented Mar 2, 2019

post_modified and post_modified_gmt are explicitly set to "now"
see commit ff504f7
therefore, the dates won't be exactly the same due to processing time and the sleep(1) above

@candrews
Copy link
Copy Markdown
Contributor Author

candrews commented Mar 2, 2019

@borekb This hopefully helps that release process :)

@borekb borekb requested review from JanVoracek and borekb March 4, 2019 14:44
// see commit ff504f77
// therefore, the dates won't be exactly the same due to processing time and the sleep(1) above
$this->assertEquals(new DateTime($clonedModifiedDate), new DateTime($modifiedDate), '', 5);
$this->assertEquals(new DateTime($clonedModifiedDateGmt), new DateTime($modifiedDateGmt), '', 5);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure this is correct. The point of the merge driver is to make both dates the same after merge, and the merge happens above on line 130. Here's a snippet from the merge driver:

// Replace date value in both files to be more recent
if ($aDate->getTimestamp() > $bDate->getTimestamp()) {
$bFile = preg_replace($dateReplacePattern, '${1}' . $aDateString . '${3}', $bFile);
} else {
$aFile = preg_replace($dateReplacePattern, '${1}' . $bDateString . '${3}', $aFile);
}

So at this point, $clonedModifiedDate should be equal to $modifiedDate. Is that correct, @JanVoracek?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like that behavior changed in ff504f7 so the date should be "now" and not whatever it was before

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@borekb: Yes, it is correct, I guess. And notice that test works for me when PHP implementation of merge driver is forced. But test is using Bash implementation by default and there is probably some hidden problem.

@borekb
Copy link
Copy Markdown
Member

borekb commented Mar 4, 2019

@borekb This hopefully helps that release process :)

Thanks, yes, this is one of the biggest blockers for the release. Thanks for starting the PR.

@candrews candrews mentioned this pull request Mar 4, 2019
@pavelevap
Copy link
Copy Markdown
Collaborator

I created separate issue for this failing test, see #1382.

I agree with @borekb that this problem should lead to the same date after merge and this PR probably does not solve it.

@borekb
Copy link
Copy Markdown
Member

borekb commented Mar 5, 2019

We think we found a true cause of the failing test – our merge driver doesn't work on certain Linux distros, #1384. The test itself is fine so I'm going to close this.

@borekb borekb closed this Mar 5, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants