wp-now: Add tests that check resulting fs in different modes #339
Conversation
wojtekn
requested review from
danielbachhuber,
adamziel,
kozer,
sejas and
katinthehatsite
| /** | ||
| * Copy example directory to a temporary directory | ||
| * | ||
| * Also, silent the console.log() output. |
There was a problem hiding this comment.
Maybe we should have a helper function for our log output, so we don't have to suppress the entirety of console.log() in tests?
There was a problem hiding this comment.
Do you mean adding a condition on the implementation side to use console.log() only if the code is executed outside the test context?
There was a problem hiding this comment.
I've checked it more, and I found out that jest has a --silent option built-in. We can run this one for CI and normal test execution, and skip the flag if we need to see the output, e.g. for test debugging purposes.
nx test wp-now --silent
That way we wouldn't need to suppress it in tests code, and we wouldn't need to hardcode hiding output for jest context in the tool code.
There was a problem hiding this comment.
Finally, I've used a wrapper for console, which does nothing for the test environment.
| */ | ||
| beforeEach(() => { | ||
| const tmpDirectory = os.tmpdir(); | ||
| const directoryHash = crypto.randomBytes(20).toString('hex'); |
There was a problem hiding this comment.
Let's add a wp-now-tests- prefix to the directoryHash so it's somewhat obvious where these directories are coming from.
There was a problem hiding this comment.
Good idea, I've added the flag. Note that we are removing the directory in afterEach, so ideally they shouldn't stay in /tmp after tests are executed. It is still a good change for cases in which the test crashes and doesn't run the cleanup method.
| await startWPNow(rawOptions); | ||
|
|
||
| const finalProjectContent = fs.readdirSync(projectPath); | ||
| expect(initialProjectContent).toEqual(finalProjectContent); |
There was a problem hiding this comment.
Can we use explicit assertions in this test?
When the expected results are stored in a variable, it's difficult to evaluate what the test is actually doing.
| 'wp-content/mu-plugins/0-allow-wp-org.php' | ||
| ]; | ||
|
|
||
| expectRequiredFiles(requiredFiles, wpNowOptions.documentRoot, php); |
There was a problem hiding this comment.
Are there any files we don't expect to be placed in the project directory? Can we add tests for them?
There was a problem hiding this comment.
The expectEmptyMountPoints function will ensure that mount points are empty directories, so those files can't exist if that part passes. Do you have any other file in mind?
There was a problem hiding this comment.
Oh, as an example, that wp-config.php doesn't accidentally exist in the project directory. It'd probably be good to have assertions for each file that wp-now might touch.
|
|
||
| await startWPNow(rawOptions); | ||
|
|
||
| expect(fs.readdirSync(projectPath)).toEqual( |
There was a problem hiding this comment.
I think it'd be better to have explicit assertions here, instead of reading data and assigning it to a variable.
There was a problem hiding this comment.
I already changed it - I'm not assigning those to variables but comparing the content of two paths.
There was a problem hiding this comment.
Sure, but it's still variable data. I think it should be static, otherwise it may break unexpectedly.
There was a problem hiding this comment.
@danielbachhuber so would you use an array of paths for each toEqual() call here, instead of reading example directories?
| return process.env.NODE_ENV !== 'test'; | ||
| } | ||
|
|
||
| export const output = shouldOutput() ? console : null; |
There was a problem hiding this comment.
I love it, thank you! Later on let's export a function to disable that manually for the VS Code extension
There was a problem hiding this comment.
Sounds good, we may also disable the output for the PHP command we are adding in #242
What?
In this PR, I propose adding tests that check the resulting filesystem (mount points, files available for the PHP server) for different modes.
Why?
We want to establish requirements for filesystem behavior in different modes and ensure that subsequent PRs won't introduce any issues.
See #312
How?
CI executes tests so we will be alerted for any PR breaking current modes behavior.
Testing Instructions
Run tests: