-
-
Notifications
You must be signed in to change notification settings - Fork 616
test: Run fixture-based scenarios in isolated directories #1657
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
So that we see the behat command output if it fails, making it easier than trying to debug why file contents do not match expectations.
These assertions were checking for deletion of the wrong file paths for the scenario. Although the step is worded "should have been removed" it is in practice only checking that the file does not exist - and therefore was passing because the files had never been created. Updated to the correct names of the files.
Previously, we were running the scenario in the fixture directory itself (or explicitly copying individual files to a temporary directory). This risks Behat modifying fixture files during the scenario (potentially unexpectedly) and therefore affecting subsequent scenarios. Instead, run every scenario in an isolated temporary working directory, and simply initialise this from the fixture at the beginning. This guarantees that every scenario runs in an isolated environment. It also simplifies our step definitions, as we no longer need to provide for copying individual files to a temp dir or having separate steps to assert the content / presence of files in the temp dir. Placeholders to some paths in features have changed because the paths are now relative to the %%WORKING_DIR%% placeholder (and are not nested under the fixture directory path) rather than to the project root.
Now that we have symfony/filesystem in dev, we can use it to simplify creating directories and files during our own behat execution. This also provides for more robust handling of unexpected error cases. Using symfony/filesystem to clean up our temporary test directories also fixes a bug that the previous ::clearDirectory method was only removing files within the temporary directories - the directories themselves were never removed.
| Then the output should contain: | ||
| """ | ||
| Scenario: # <href=phpstorm://open?file={BASE_PATH}tests/Fixtures/EditorUrl/features/test.feature&line=3>features/test.feature:3</> | ||
| Scenario: # <href=phpstorm://open?file=%%WORKING_DIR%%features/test.feature&line=3>features/test.feature:3</> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As a separate PR, I might look at standardising / simplifying the way our FeatureContext handles the various placeholders for directories and filesystem paths - they are not quite consistent with each other, and I think some might be redundant.
But for the purpose of this PR, all paths in features will now be relative to the (previously defined) %%WORKING_DIR%% placeholder.
|
I'll take a look at the directory separator and path issues on mac/windows tomorrow. |
Pending review of the best way to do this consistently / reliably across all the features, migrate these ones to the %%DS%% placeholder for now.
The existing `"foo.php" file should contain` step passes the actual file content through the `getExpectedOutput()` function that is also used when asserting command output. This function automagically converts directory separators in paths into the DIRECTORY_SEPARATOR for the platform, as well as replacing various other placeholders. This fails when trying to assert the content of files generated by the convert-config command with any `removePrefix` paths configured. The generated files contain paths with the literal `/` character (matching the content of the YAML) on all platforms. Instead, assert the exact content of these files, only normalising newlines. Again pending review of how we consistently / reliably handle path and directory separator issues across all our scenarios.
On MacOS and Windows runners, the path returned by sys_get_temp_dir (and stored in the workingDir variable) is not a real path. Therefore, it does not match when passed as an option (e.g. for --remove-prefix) into Behat. We need to convert to a realpath so that it matches the actual path to the files on disk.
|
@carlos-granados this should be good to go now. I will definitely take a look at how we specify / assert paths and directory separators etc in our features as a separate PR once this is merged - there's a fair bit of inconsistency that's crept in over the years and I think we could be clearer and more robust. |
Since scenarios all run in a temporary working directory, and the workingDir and tempDir are now the same, we can simplify features, steps, and placeholders by only referring to the workingDir.
e769740 to
ca9d65d
Compare
carlos-granados
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @acoulton, great job, just a tiny comment, feel free to ignore it
Per code review.
Previously, we were running the scenario in the fixture directory itself (or explicitly copying individual files to a temporary directory). This risks Behat modifying fixture files during the scenario (potentially unexpectedly) and therefore affecting subsequent scenarios.
Instead, run every scenario in an isolated temporary working directory, and simply initialise this from the fixture at the beginning. This guarantees that every scenario runs in an isolated environment.
It also simplifies our step definitions, as we no longer need to provide for copying individual files to a temp dir or having separate steps to assert the content / presence of files in the temp dir.
Placeholders to some paths in features have changed because the paths are now relative to the %%WORKING_DIR%% placeholder (and are not nested under the fixture directory path) rather than to the project root.
This PR also adds some small refactors and enhancements to our features & FeatureContext - including fixing a bug that we were not cleaning up temporary directories created while running our features.