Testing: Test Create Block with more OS and Node versions #38368
Conversation
gziolo
force-pushed
the
update/ci-create-block-os-node-version
branch
from
ad78354 to
0c08f5d
Compare
gziolo
added
[Type] Automated Testing
|
It would be also great to include a check in the bash script that verifies if all files were build for the block using gutenberg/bin/test-create-block.sh Lines 31 to 32 in 32f35ce @ockham, I guess |
|
Does anyone know how do I run the test script on Windows OS. It fails at the moment:
More in https://github.com/WordPress/gutenberg/runs/5003897537?check_suite_focus=true. |
Hmm, maybe try something like expected=57
actual=$( ls build | wc -l | xargs )
if [ "$expected" != "$actual" ]; then
exit 1
fi(The |
Hmm, this is a tricky one, since there are two levels of indirection:
It's the latter line that Windows seems to be struggling with; or rather, the shell that Windows is using. My first attempt at a solution would thus be to set the shell on Windows to bash (where the default would be Powershell). Something like diff --git a/.github/workflows/create-block.yml b/.github/workflows/create-block.yml
index 61ae4c49be..37044229f4 100644
--- a/.github/workflows/create-block.yml
+++ b/.github/workflows/create-block.yml
@@ -36,6 +36,7 @@ jobs:
cache: npm
- name: npm install, build, format and lint
+ shell: bash
run: |
npm ci
npm run test:create-block |
I know there's also been some use of |
Try enforcing shell bash when testing the block
Brilliant, it works like a charm. This is what I ened up using: expected=5
actual=$( ls build | wc -l | xargs )
if [ "$expected" != "$actual" ]; then
error "Expected $expected files in the build folder, but found $actual."
exit 1
fi |
|
I'm afraid that the following trick didn't help: shell: bashI spend some time on StackOverflow, but I didn't find anything useful so far. It looks like the whole problem is related to |
Yeah, that's my hunch as well, I'm afraid. I've seen some threads on SO that referred to a very similar problem -- having a For those instances, the suggested workaround was to explicitly use since node itself apparently does accept the leading-dot and foward-slash notation for the file it's passed as a cmd line arg, even on Windows. But alas, that doesn't translate to our case, where we're dealing with a (POSIX) shell script. I've seen some people suggest prepending That seems to work across some platforms, but from what I've seen, maybe not on Windows (it's worth trying it out tho -- maybe it doesn't work with PowerShell but does with bash?) If all else fails, how about inlining the contents of |
I pushed 319a6bc to try this out. |
|
Size Change: +1.36 kB (0%) Total Size: 1.14 MB
ℹ️ View Unchanged
|
|
It looks like it works. The failure still happens on Windows but in the newly added verification for the build folder. |
Ah yeah, so I guess that's at least some good news 🎉
Indeed: 🤔 |
Documented here. Maybe we can work around using |
|
With some further digging into StackOverflow, I found |
| @@ -31,6 +35,14 @@ status "Formatting files..." | |||
| status "Building block..." | |||
| ../node_modules/.bin/wp-scripts build | |||
|
|
|||
| status "Verifying build..." | |||
| expected=5 | |||
| actual=$( ls build | wc -l | awk '{$1=$1};1' ) | |||
There was a problem hiding this comment.
awk '{$1=$1};1'
Mysterious incantation! 😄
OK, I think I got it: is the issue that, because wc -l formats the output with a leading space, the comparison [ "$expected" != "$actual" ] always fails? There are a couple of ways to make this clearer:
- Since we're already invoking AWK, we can let it do the counting too:
awk 'END { print NR }', which means "once the end of the file/stream is reached, print the number of records, i.e. lines, that we have processed. Alternatively, grepcan also count:grep -c .` (the dot represents any character to match) - But, actually, Bash is capable of arithmetic comparison:
[ "$expected" -ne "$actual" ], where-nestands for "not equal". In this scenario, Bash will parse the numbers unencumbered by formatting.
| run: | | ||
| npm ci | ||
| npm run test:create-block | ||
| sh ./bin/test-create-block.sh |
There was a problem hiding this comment.
Unless there is a GitHub Actions quirk that I am not aware of, in a normal environment this invocation is problematic, because test-create-block.sh isn't a POSIX shell script, but a Bash script with specific Bashisms. So it should be:
bash ./bin/test-create-block.sh
I'll open a follow-up PR.
There was a problem hiding this comment.
(FWIW, we're setting the shell to bash, so in practice, it shouldn't matter much IIUC.)
There was a problem hiding this comment.
(FWIW, we're setting the shell to
bash, so in practice, it shouldn't matter much IIUC.)
Indeed, but I don't understand what that actually means, mechanically. :) That is, in the end in the runtime there must be something equivalent to exec, and this could either mean that some layer is honouring the hashbang (parsing the first line of the script — #!/bin/bash — and thus exec'ing /bin/bash with the script path as argument), or that some layer is directly exec'ing entire string in the config — bash ./bin/test-create-block.sh. I have no idea what kind of effect shell: bash plays in the middle of this!
There was a problem hiding this comment.
BTW, coming back to this from another angle: Are there even that many Bashisms left? Seems like -ne could be actually POSIX-compliant 🤔
There was a problem hiding this comment.
Actually, the -ne is fine and dates back to the original Unix days, I believe. :) According to ShellCheck, the only Bashism is the -e flag passed to echo. Replacing echo -e with printf should be enough — just don't forget the trailing newline.
Btw, ShellCheck is available as a linter. I have it in Vim, so I'm sure it's trivial to install in VSCode. I highly recommend it. It not only prevents mistakes, but it has also taught me a lot thanks to the detailed explanations in the wiki. :)
There was a problem hiding this comment.
Thanks for the reminder! I was still copying code back and forth into shellcheck.net. (I have yet to try if it's clever enough to work inside of the workflow YAML 🤔 )
|
Follow-up at #38482! :) |
Description
We had a regression in #38348 for Windows OS when using
@wordpress/scripts. That could be caught early in the process with GitHub Actions if we were testing more operating systems. This PR tries to include Windows and macOS when testing Create Block.It also adds Node 16 to the mix.(Node 16 doesn't work with the current lock file because it uses npm 8)