Enable spvc test. #578
Enable spvc test. #578
Conversation
Update DEPS to work with spirv-cross tests. Fix spvc return status. Simplified the test script: - no caching of compiled shaders - all testing steps in one place - removed unsupported spvc flags Also clean up temporary files so they can't mess with subsequent testing.
| if not (exc.errno == errno.EEXIST and os.path.isdir(dirpath)): | ||
| raise | ||
| def check_call(cmd): | ||
| global devnull |
There was a problem hiding this comment.
I dislike backdoor declarations of global variables. It was better to have devnull and tmpfile at global scope.
(I didn't even know Python could do this. Seems.... unpythonic?)
spvc/test/run_spirv_cross_tests.py
Outdated
| global devnull | ||
| if subprocess.call(cmd, stdout=devnull): | ||
| print('failed command: ', ' '.join(cmd)) | ||
| sys.exit(1) |
There was a problem hiding this comment.
Either return an error or throw an exception.
There was a problem hiding this comment.
Whoops, that was a debugging print I meant to remove.
This will call subprocess.check_call and throw an exception.
| except OSError as exc: | ||
| if not (exc.errno == errno.EEXIST and os.path.isdir(dirpath)): | ||
| raise | ||
| def check_call(cmd): |
There was a problem hiding this comment.
The methods should have method comments.
This became acute for me when reading the spvc function (since this version of the code has mixed responsibility: to run the command and increment the test count.)
spvc/test/run_spirv_cross_tests.py
Outdated
| def do_test(binary_path, reference_path, flags): | ||
| global test_count, pass_count | ||
| global devnull | ||
| global test_count | ||
| test_count += 1 |
There was a problem hiding this comment.
I would not modify the test counter inside this utility function. That mixes responsibilities.
| except: | ||
| pass | ||
|
|
||
| def test_glsl(shader, filename, optimize): |
There was a problem hiding this comment.
Very much needs a method comment.
This method name has glsl in it, but it's definitely handling SPIR-V assembly cases, etc.
Badly named method?
There was a problem hiding this comment.
This script or this method should describe that it's tracking the behaviours of SPIRV-Cross' own testing tactics. Give the next person a place to look in SPIRV-Cross itself when there's new or changed behaviour to be replicated.
There was a problem hiding this comment.
I expanded the comments. The function could be split up, but there's also something to be said for not having to page up and down looking for functions. Instead I've tried to clearly label the three main steps with comments.
spvc/test/run_spirv_cross_tests.py
Outdated
| output = "" | ||
| if '.vk.' in filename or '.asm.' in filename: | ||
| output_vk = tmpfile + 'vk' + filename | ||
| if spvc(tmpfile, output_vk, flags): |
There was a problem hiding this comment.
I can see why you are tempted to put the test counting in the low level utility function. Probably because there's so much business logic in constructing the right command lines.
I'd approach it differently. I'd construct the elaborated list of command lines, then execute them one by one, counting at that point. That would also give an easy way to add a -n or --dry-run option to preview exactly what's going to execute before it's executed.
That also would resolve the usability problem where: when something goes wrong, it's annoying to have to guess how to reconstruct exactly one test case for iterative fix/debug.
There was a problem hiding this comment.
It's a little more complicated to separate listing the steps and running through them.
If spvc fails you need to skip the comparison step. The step could maybe be "spvc then compare if needed" but there's a property here I want to preserve, namely doing everything in the same order as spirv-cross. That way I can log what both are doing and easily compare. I had logging here temporarily and removed it - will add later as an official feature. That also addresses the issue of knowing what went wrong (tail the log). The logging in spirv-cross was similarly temporary and I'll try to land it there eventually.
Ideally some day we get rid of this duplication, but I'm proceeding cautiously on trying to change spirv-cross.
I think this is about as simple as possible at the moment, and that's a good thing. I'd like to flesh out the MSL and HLSL testing a bit and see what complications that brings before deciding on another direction for this script.
Add comments. Introduce all globals in global scope. Rearrange code slightly.
Includes 7425820 Start v2019.1-dev 34c412f Finish v2019.0 7e46a09 Update CHANGES 63ab5ac Re-write environment control flags to be consistent (google#637) 538a9d2 Add ability to transform spvc input from WebGPU SPIR-V (google#631) 0b84e66 Ran 2to3 on git-sync-deps (google#636) 27bffdf Roll spirv-tools dependency (google#634) c7b4974 Remove dead file (google#632) 5ba891a Convert spirv-cross tests to using a known failures list (google#626) 05c766a Add flag to allow filtering tests cases being run (google#625) 9f04be0 Run pyformat on utils and test scripts (google#624) 326165c Run spirv-cross tests in parallel (google#622) 08549a9 Convert to requiring Python 3 (google#621) 1475418 Change 'target' env to 'source' env in spvc API (google#610) dce7a4b Roll glslang, spirv-tools, spirv-headers, spirv-cross (google#614) 5cf297d Spvc tests: Assemble explicitly for Vulkan1.1 env (google#615) 1defed7 Fix spvc test logging (google#616) 3ac606a Update CHANGES, switch to 2019.0-dev (google#613) 55b9f5d 1.4: GOOGLE suffix drops from certain instructions f51af41 Fix typos in the artifacts links. (google#606) afc69d3 Disable running re2 tests (google#603) 6805e55 Fix Windows build uploads. (google#597) 60caf55 Package and upload builds. (google#595) 59a49bc Shaderc requires Python3 (google#594) 5af3dbf Roll spirv-cross. (google#592) ff9ae40 Satisfy gn check. (google#591) 60658b0 Fix gn build. (google#590) 439a848 Export config files for pkg-config. (google#581) 2caa40b spvc: Add MSL and HLSL tests. (google#585) 1d9383f Roll DEPS. (google#586) 7bb8a43 spvc testing: add logging. (google#583) 067a749 spvc: Add some flags. (google#579) f7fa8ce Enable spvc test. (google#578) 4b8446f Upate python scripts work with python3. (google#569) eb0e335 Add presubmit_clang_asan bot. (google#575) 665bbc1 Add spvc command line tool and tests. (google#571) 337d42d Remove extra .clang-format files. (google#573) cfb65d4 Fix spvc_c_smoke_test some more. (google#574) 712cebf Fix spvc_c_smoke_test. (google#572) 6bba7fe Avoid leading dot slash in cmake target. (google#570) 94f21e8 Roll GLSLang. (google#564) Testing: checkbuild.py on Linux; unit tests on Windows Change-Id: Ie1b63aacac7194fe8330068e6a01c5bdf44cd321
Update DEPS to work with spirv-cross tests.
Fix spvc return status.
Simplified the test script:
Also clean up temporary files so they can't mess with subsequent testing.