Tests: make FileCheck a substitution that sanitizes the input #4187

gribozavr · 2016-08-10T06:30:20Z

In the Swift testsuite, FileCheck is used to check for (or for absence of) fixed strings in the input. It is not an issue when the string is long and complex (e.g., a full SIL instruction). But sometimes the string is a common word (e.g., 'sdk' or 'branch'). Tests that use such patterns are known break when the path to the build directory contains these words.

This change makes %FileCheck a lit substitution that replaces paths to source and build directories with fixed strings.

Before merging this pull request to apple/swift repository:

Test pull request on Swift continuous integration.

Triggering Swift CI

The swift-ci is triggered by writing a comment on this PR addressed to the GitHub user @swift-ci. Different tests will run depending on the specific comment that you use. The currently available comments are:

Smoke Testing

Platform	Comment
All supported platforms	@swift-ci Please smoke test
All supported platforms	@swift-ci Please smoke test and merge
OS X platform	@swift-ci Please smoke test OS X platform
Linux platform	@swift-ci Please smoke test Linux platform

A smoke test on macOS does the following:

Builds the compiler incrementally.
Builds the standard library only for macOS. Simulator standard libraries and
device standard libraries are not built.
lldb is not built.
The test and validation-test targets are run only for macOS. The optimized
version of these tests are not run.

A smoke test on Linux does the following:

Builds the compiler incrementally.
Builds the standard library incrementally.
lldb is built incrementally.
The swift test and validation-test targets are run. The optimized version of these
tests are not run.
lldb is tested.

Validation Testing

Platform	Comment
All supported platforms	@swift-ci Please test
All supported platforms	@swift-ci Please test and merge
OS X platform	@swift-ci Please test OS X platform
OS X platform	@swift-ci Please benchmark
Linux platform	@swift-ci Please test Linux platform

Lint Testing

Language	Comment
Python	@swift-ci Please Python lint

Note: Only members of the Apple organization can trigger swift-ci.

gribozavr · 2016-08-10T06:30:38Z

@tkremenek Would you mind reviewing?

gribozavr · 2016-08-10T07:09:50Z

@swift-ci Please test

jrose-apple · 2016-08-10T15:56:39Z

If FileCheck is going to be a substitution, it really should have %. That can be a separate commit, but I don't think it should be a separate PR.

jrose-apple · 2016-08-10T15:57:23Z

Also, what if the path to the source dir contains regex metacharacters, or @ signs?

gribozavr · 2016-08-10T16:19:48Z

If FileCheck is going to be a substitution, it really should have %. That can be a separate commit, but I don't think it should be a separate PR.

This is a massive change to all tests, is it actually worth it? I tried making it, but then ended up reverting it because the resulting patch was huge and the percent sign didn't seem provide enough benefit for the reader of the RUN line. Seeing %FileCheck, you would know that it is not the real FileCheck, but is that important? Especially since we want this substitution to be used in all tests by default, and only skipping the sanitization step when the test has a certain unusual structure.

I don't have a strong preference here, just wanted to hear your thoughts about it.

gribozavr · 2016-08-10T16:20:03Z

Also, what if the path to the source dir contains regex metacharacters, or @ signs?

Good point. I'll escape them.

jrose-apple · 2016-08-10T16:35:55Z

I feel very strongly that anything without a % should work when you write the equivalent thing on the command line. The way we get around the "raw" bit is to have another substitution for " FileCheck " that produces an error; you can see that in action in lit.cfg. If someone really wants the raw one, they can then quote it.

rjmccall · 2016-08-10T16:52:06Z

lit already does some pretty serious command-line parsing, I think; I don't think it's too weird for it to substitute "plaintext" commands, especially since that will be reflected in its logged command lines. Although maybe we should ask @ddunbar.

If we create a subtle difference between FileCheck and %FileCheck, we're just going to end up with this bug duplicated incrementally.

jrose-apple · 2016-08-10T16:57:00Z

I don't think that's true. lit's command-line parsing has only gotten more shell-like over the years. And nobody complains about a subtle difference between clang and %clang, because the first one produces an error.

rjmccall · 2016-08-10T17:12:42Z

That's not a subtle difference, then.

jrose-apple · 2016-08-10T17:21:34Z

Right, as I said above we should do the same for FileCheck.

ddunbar · 2016-08-10T17:53:03Z

My $0.02:

I am ok with something like FileCheck not being a substitution, if it is simply phrased as how lit does the executable search for the first argument. I agree with Jordan the command lines should replay... it would be nice for lit to resolve the first argument to a complete path, and then show that in the output.

I already wanted such a mechanism in place to solve the hacks that LLVM does for finding its tools in a more principled manner. It should just be possible for the lit config to define a function which maps executable names to arguments.
For this particular problem, it feels like it should be fixed at the LLVM level not just worked around via a shim script. Maybe one option would be to add FileCheck options for the sanitization, and then use the mapping mechanism from Redundant Load Elimination Patches #1 to automatically provide them?

jrose-apple · 2016-08-10T18:04:07Z

Ah, I'm actually against expanding FileCheck just for path purposes (@gottesmm and I have had this argument several times). If we're already using some other substitution, though, then doing that as well doesn't make it any worse.

ddunbar · 2016-08-10T18:07:20Z

What is your objection to expanding FileCheck for path purposes (I assume that equals meaning using a custom PATH search for it)? LLVM already does that all over the place for llc, etc.?

jrose-apple · 2016-08-10T19:00:00Z

I like being able to read both the initial RUN lines and what lit spits out. But maybe I lost that battle already.

atrick · 2016-08-10T20:38:17Z

FWIW I very frequently need to read and understand the RUN line's full expansion. I'm constantly manually breaking tests just to find out what the actual run line is. I don't want that process to get any worse.

jrose-apple · 2016-08-10T20:47:34Z

For that last, I'd actually like to add a mode to lit where it just dumps run lines and doesn't run tests. checks --help output Oh, -a outputs command lines on success, but you can't combine it with --no-execute.

atrick · 2016-08-10T20:50:55Z

Debugging a failure for me usually starts with understanding exactly how the test was invoked. I often want to reduce it to a smaller command line, smaller test case, and run lldb.

ddunbar · 2016-08-10T20:51:29Z

That would be a great idea, and shouldn't actually be very hard.

The existing --no-execute isn't very useful (I used it for perf timing, but that doesn't deserve that command line arg spelling).

jrose-apple · 2016-08-11T00:15:32Z

utils/PathSanitizingFileCheck

+    stdin = sys.stdin.read()
+    for s in args.sanitize_strings:
+        replacement, pattern = s.split('=', 1)
+        stdin = stdin.replace(pattern, replacement)


Seems expensive to do this in-memory rather than via pipe, but we can add that later.

gribozavr · 2016-08-11T01:07:40Z

@jrose-apple I have addressed your comments:

We use pipes instead of temporary files now.
I switched test/Interpreter/SDK/Foundation_NSLog.swift to %FileCheck (instead of the raw one).

gribozavr · 2016-08-11T01:18:37Z

@swift-ci Please test

jrose-apple · 2016-08-11T01:19:56Z

The substitution is still in-memory, but that's probably fine for now. Thanks, Dmitri!

tkremenek · 2016-08-11T05:02:51Z

Just caught up on this. I think this is a great addition.

gribozavr · 2016-08-11T05:35:50Z

OS X failed with a SwiftPM failure in PackageLoadingTests.ManifestTests testManifestLoading. Retrying.

@swift-ci Please smoke test OS X platform

gribozavr · 2016-08-11T06:21:06Z

@swift-ci Please smoke test OS X platform

gribozavr · 2016-08-11T06:21:16Z

@swift-ci Please test OS X platform

gribozavr · 2016-08-11T06:50:17Z

SceneKit_test.swift failures look unrelated. Will retry tomorrow, maybe the CI will be in a better state.

'%FileCheck' removes absolute paths of the source and build directory from the input. Overwhelming majority of tests don't intend to match these paths. Also add a substitution '%raw-FileCheck' that does not sanitize the input.

This reverts commit 02039a1. With the new `%FileCheck` substitution that removess full paths to the build directory from FileCheck inputs, this workaround is no longer needed.

gribozavr · 2016-08-11T07:04:34Z

Resolved conflicts and rebased. Retrying.

gribozavr · 2016-08-11T07:05:31Z

@swift-ci Please smoke test OS X platform

gribozavr · 2016-08-11T07:05:45Z

@swift-ci Please test OS X platform

gribozavr · 2016-08-11T07:19:25Z

Tested locally on Linux, tests pass.

gribozavr · 2016-08-11T15:32:26Z

macOS passed on CI, merging!

Resolve conflicts with upstream `main`

gribozavr force-pushed the FileCheck-substitution branch from be0e85b to cc79e5c Compare August 10, 2016 06:33

gribozavr force-pushed the FileCheck-substitution branch from cc79e5c to 123e076 Compare August 11, 2016 00:12

jrose-apple reviewed Aug 11, 2016
View reviewed changes

gribozavr force-pushed the FileCheck-substitution branch from 279a2dc to 6d313b0 Compare August 11, 2016 01:06

rintaro mentioned this pull request Aug 11, 2016

[lit] Show test script when '--no-execute -a' is specified to lit.py #4221

Closed

1 task

gribozavr added 4 commits August 10, 2016 23:50

test/lit.cfg: add a new substitution '%FileCheck'

7284caf

'%FileCheck' removes absolute paths of the source and build directory from the input. Overwhelming majority of tests don't intend to match these paths. Also add a substitution '%raw-FileCheck' that does not sanitize the input.

Fix tests to work with the new 'FileCheck' substitution

dcadab0

Migrate FileCheck to %FileCheck in tests

d175b3b

Revert "Fix copyforward test - rename branch to xbranch"

94c7c3a

This reverts commit 02039a1. With the new `%FileCheck` substitution that removess full paths to the build directory from FileCheck inputs, this workaround is no longer needed.

gribozavr force-pushed the FileCheck-substitution branch from 6d313b0 to 94c7c3a Compare August 11, 2016 07:04

gribozavr merged commit a33203f into master Aug 11, 2016

gribozavr deleted the FileCheck-substitution branch August 11, 2016 15:32

gribozavr mentioned this pull request Aug 11, 2016

[swift-3.0-branch] Tests: make FileCheck a substitution that sanitizes the input #4228

Merged

kateinoigakukun pushed a commit that referenced this pull request Aug 31, 2022

Merge pull request #4187 from swiftwasm/maxd/main-merge

a9e0106

Resolve conflicts with upstream `main`

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tests: make FileCheck a substitution that sanitizes the input #4187

Tests: make FileCheck a substitution that sanitizes the input #4187

gribozavr commented Aug 10, 2016 •

edited

Loading

gribozavr commented Aug 10, 2016

gribozavr commented Aug 10, 2016

jrose-apple commented Aug 10, 2016

jrose-apple commented Aug 10, 2016

gribozavr commented Aug 10, 2016

gribozavr commented Aug 10, 2016

jrose-apple commented Aug 10, 2016

rjmccall commented Aug 10, 2016

jrose-apple commented Aug 10, 2016

rjmccall commented Aug 10, 2016

jrose-apple commented Aug 10, 2016

ddunbar commented Aug 10, 2016 •

edited

Loading

jrose-apple commented Aug 10, 2016

ddunbar commented Aug 10, 2016

jrose-apple commented Aug 10, 2016

atrick commented Aug 10, 2016

jrose-apple commented Aug 10, 2016

atrick commented Aug 10, 2016

ddunbar commented Aug 10, 2016

jrose-apple Aug 11, 2016

gribozavr commented Aug 11, 2016

gribozavr commented Aug 11, 2016

jrose-apple commented Aug 11, 2016

tkremenek commented Aug 11, 2016

gribozavr commented Aug 11, 2016

gribozavr commented Aug 11, 2016

gribozavr commented Aug 11, 2016

gribozavr commented Aug 11, 2016

gribozavr commented Aug 11, 2016 •

edited

Loading

gribozavr commented Aug 11, 2016

gribozavr commented Aug 11, 2016

gribozavr commented Aug 11, 2016

gribozavr commented Aug 11, 2016

Tests: make FileCheck a substitution that sanitizes the input #4187

Tests: make FileCheck a substitution that sanitizes the input #4187

Conversation

gribozavr commented Aug 10, 2016 • edited Loading

gribozavr commented Aug 10, 2016

gribozavr commented Aug 10, 2016

jrose-apple commented Aug 10, 2016

jrose-apple commented Aug 10, 2016

gribozavr commented Aug 10, 2016

gribozavr commented Aug 10, 2016

jrose-apple commented Aug 10, 2016

rjmccall commented Aug 10, 2016

jrose-apple commented Aug 10, 2016

rjmccall commented Aug 10, 2016

jrose-apple commented Aug 10, 2016

ddunbar commented Aug 10, 2016 • edited Loading

jrose-apple commented Aug 10, 2016

ddunbar commented Aug 10, 2016

jrose-apple commented Aug 10, 2016

atrick commented Aug 10, 2016

jrose-apple commented Aug 10, 2016

atrick commented Aug 10, 2016

ddunbar commented Aug 10, 2016

jrose-apple Aug 11, 2016

Choose a reason for hiding this comment

gribozavr commented Aug 11, 2016

gribozavr commented Aug 11, 2016

jrose-apple commented Aug 11, 2016

tkremenek commented Aug 11, 2016

gribozavr commented Aug 11, 2016

gribozavr commented Aug 11, 2016

gribozavr commented Aug 11, 2016

gribozavr commented Aug 11, 2016

gribozavr commented Aug 11, 2016 • edited Loading

gribozavr commented Aug 11, 2016

gribozavr commented Aug 11, 2016

gribozavr commented Aug 11, 2016

gribozavr commented Aug 11, 2016

gribozavr commented Aug 10, 2016 •

edited

Loading

ddunbar commented Aug 10, 2016 •

edited

Loading

gribozavr commented Aug 11, 2016 •

edited

Loading