Conversation
|
Please merge master into this and resolve conflicts. |
3f7d076 to
ff8eee7
Compare
ff8eee7 to
02dd1be
Compare
|
Should I perform LoadTest statistics also for this version of driver? |
|
@octopuss I think we should (you on Mac, I'll do it on Windows). |
|
|
||
| done | ||
|
|
||
| # Place placeholder between lines |
There was a problem hiding this comment.
- "... to avoid merge conflicts on adjacent lines" (to make the comment a bit more descriptive)
|
(The commit history is messed up here but let's leave it like that, it's not a big deal.) |
|
I'm looking at |
| $branchDate = '17-02-16 19:19:23'; | ||
|
|
||
| MergeDriverTestUtils::fillFakeFileAndCommit($originDate, 'file.ini', 'Initial commit to Ancestor'); | ||
| MergeDriverTestUtils::fillFakeFileAndCommit('10-02-16 08:00:00', 'file.ini', 'Initial commit to Ancestor'); |
There was a problem hiding this comment.
What's fake about the file? Seems like a normal test file to me. Is it faking anything?
There was a problem hiding this comment.
To me it is faking ini files created by VersionPress. There is no guid at the top of created file. But you are right that the file created is legit for the purpouse of this test.
- change method name
…ether to better track timeline.
… attribute to be more explicit what it does.
b15c161 to
7a82a21
Compare
- MergeDriverTests now use more narrative style – the sequence of steps is directly visible in the test methods - Two new asserts for asserting clean and conflicting merges - Some methods in MergeDriverTestUtils renamed / refactored
|
I'll do some more updates / refactorings:
|
…obal constants were removed (refactored). This also influenced tests. Details: - MergeDriverInstaller::installMergeDriver() method now has a $driver parameter through which the code can force the implementation. - Tests use this $driver parameter so we could get rid of the brittle `switchDriverToBash()` and `switchDriverToPhp() methods in MergeDriverTestUtils. - MergeDriverInstaller no longer uses global constants like VP_PROJECT_ROOT or VERSIONPRESS_MIRRORING_DIR. The code is more explicit and self-contained now. - Test were slightly refactored again to remove some unnecessary code.
|
Guys, could you please review my recent commits when you have time? In the meantime I'll fix the remaining two issues (failing test on Windows and more unique placeholder). |
… be used in some post quite easily)
2503090 to
2a81460
Compare
| } else { | ||
| $mergeDriverFile = 'ini-merge.php'; | ||
| $mergeDriverScript = '"' . PHP_BINARY . '" "' . VERSIONPRESS_PLUGIN_DIR . '/src/Git/merge-drivers/' . $mergeDriverFile . '"'; | ||
| if ($driver == 'php' || ($driver == 'auto' && DIRECTORY_SEPARATOR == '\\')) { |
There was a problem hiding this comment.
I see that you have decided not to use constants for merge driver types. I know that it is just for tests. Isn't it better to replace those strings with constants, so potential users will know exact values which can be used ?
There was a problem hiding this comment.
I treat test code the same as production code - it needs to have the same quality - so it's not an excuse here. I genuinely don't know what's better here, these are just three strings so I chose to use them directly without any "abstraction" but maybe that's just my taste. What would @JanVoracek do?
There was a problem hiding this comment.
I would probably use constants because of preventing typos.
Another note to the code: I would make the conditions simpler as the final driver can be decided much sooner.
There was a problem hiding this comment.
Ok, I'll add constants. BTW string literal types are an amazing feature of TypeScript, exactly what I'd use here if PHP supported it.
About the conditions: I had a feeling they could be simplified but it didn't occur to me how. Can you draft a pseudo-code (or real code) here? I'll happily apply it.
There was a problem hiding this comment.
Maybe something like this?
if ($driver === self::DRIVER_AUTO) {
$driver = DIRECTORY_SEPARATOR == '\\' ? self::DRIVER_PHP : self::DRIVER_BASH;
}
if ($driver === self::DRIVER_BASH) {
$mergeDriverScript = $pluginDir . '/src/Git/merge-drivers/ini-merge.sh';
chmod($mergeDriverScript, 0750);
}
if ($driver === self::DRIVER_PHP) {
$mergeDriverScript = '"' . PHP_BINARY . '" "' . $pluginDir . '/src/Git/merge-drivers/ini-merge.php' . '"';
}…ller::installMergeDriver()
|
@octopuss Please merge from master, resolve conflicts, do some final testing and merge. |
bb10448 to
f6ee3eb
Compare
|
🎉 this was a hell of a PR! Good job. |
Resolves #719. This PR is successor of PR #725 which should close #588. Important commits are only those which are related to issue #719. Other are merged PR/Branch #725.
Please review:
Thank you.
Restrictions: