Skip to content
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

Clean up test RUN commands #346

Open
2 of 13 tasks
kyleheadley opened this issue Dec 7, 2020 · 5 comments
Open
2 of 13 tasks

Clean up test RUN commands #346

kyleheadley opened this issue Dec 7, 2020 · 5 comments

Comments

@kyleheadley
Copy link
Member

kyleheadley commented Dec 7, 2020

Our tests make inconsistent use of certain features of llvm-lit. We could manually update them to be more consistent as we work towards a more general solution for future tests.

  • Add -- to the end of 3c calls (planned to be done in Add -output-dir and other file handling improvements #412)
  • Use -addcr in all 3c calls
  • Remove -fcheckedc-extension from %clang calls because it's the default (but first confirm that we don't foresee the default changing)
  • Use %clang rather than %clang_cc1 (Migrate 3C regression tests from %clang_cc1 to %clang. #596)
  • Stop having -output-postfix=checked write temporary files into clang/test/3C (3C output is not cleaned up when a test fails. #378) (planned to be done in Add -output-dir and other file handling improvements #412)
  • Redirect unused output to temp files (i.e. %t3.unused) rather than /dev/null
  • Name test expected to fail as *_xfail.c rather than *BUG.c
  • *_xfail.c tests should use these conventions as if we expect them to succeed
  • Run clang on converted files (using -f3c-tool if necessary)
  • Include a short explanation of the test
  • Include an explanation why any of the above are not relevant to a particular test
  • Avoid XFAIL in favor of asserting the specific way in which a test is expected to fail due to a known bug. (We may not get rid of all the XFAILs immediately: we may find that XFAIL has benefits in certain cases or just that taking the time to research a particular test is a low priority.)
  • If cd is used in a RUN line, it should generally be the last thing on the line (e.g., mkdir DIR && cd DIR is OK to put on one RUN line to save space, but cd DIR && SOME_OTHER_COMMAND is not). Readers may not be aware that the RUN lines form a single script in which a cd command does affect subsequent RUN lines (unless some special construct such as a subshell is used). // RUN: cd DIR && SOME_OTHER_COMMAND could mislead the reader that we needed to put both on one line because the effect of the cd would otherwise be lost. An exception to this guideline might be if a test needs a long sequence of cd DIR_A; CMD_A; cd DIR_B; CMD_B; ... and the space saving or clarity improvement of putting cd DIR_A; CMD_A on one line is significant. (Note that cd DIR_A; CMD_A is equivalent to cd DIR_A && CMD_A because lit uses the equivalent of set -e). (Guideline proposed by Matt 2021-06-21, open to debate.)

Using -- seems to suppress some output, but the test runner will do this for us. In rare cases it changes the resultant file (#343), and we never run with this option manually.

The two versions of clang are for the driver (%clang) and the front-end (%clang_cc1). The driver will call the front-end with options based on the system, so this should be the default. There may be exceptions if the front-end contains some additional debug options. One option is -verify; it can be used with %clang as -Xclang -verify.

The test generator llvm-lit provides temporary files during a test (%t, we can optionally add a suffix). Using these is preferable to creating files under clang/test/3C (which can foul up version control and future test runs), but lit does not delete %t files automatically, so we still need to delete them ourselves if we want to make sure the test starts in a predictable state and is not affected by previously existing %t files.

While /dev/null is convenient, it is not supported on windows systems. llvm-lit converts it for most cases, but not all.

Some tests do not represent our current output, but a desired future one. We mark these internally and externally with // XFAIL: *, and testname_xfail.c, respectively. They should otherwise be treated as regular tests, since we will get a notification if they succeed (say after a bugfix with wide consequences). At that point we don't want to edit them, just update the failure status.

Running clang is a good secondary test, both for the tool's correct output and for test correctness. It may not be possible in some cases, like -alltypes not yet generating array bounds for all variables. For those, use -f3c-tool, and if it still doesn't compile, leave a note about why and possibly a link to a github issue describing the error.

Explanations help in debugging or updating a test. The specific issue might need setup code that can be changed without affecting the value of the test.

This issue is large and may span multiple pull requests. It may also not all be necessary, as we evaluate our ability to automate testing.

@kyleheadley
Copy link
Member Author

I'm currently making sure that all tests call clang at least once where appropriate. Will submit pull request soon.

@mattmccutchen-cci
Copy link
Member

mattmccutchen-cci commented Dec 7, 2020

Shall we take this issue to include my proposal to refactor the per-test command lists into calls to a single script (Python for cross-platform compatibility) that accepts the needed flags? As I said at today's meeting, I think it's a waste to try to manually harmonize the tests and verify that you've done so correctly, only to go back and refactor and verify that the script executes the same commands you had written manually during the harmonization. Better to start developing the script and migrate each test to call the common script at the same time as you figure out what flags that test should pass to the script. Mike asked me to do this and I tentatively agreed, so I'll assign myself, though I don't know just how soon I can start (still dealing with some non-CCI stuff, unfortunately).

I don't care for %t, etc. because it sounds like you can't give it a meaningful name, so you have to read the commands to figure out what the file represents in any particular test. Presumably, if we write a Python script, we can develop our own way to manage temporary files and we won't be constrained by wanting the per-test command lists to be short.

@mattmccutchen-cci mattmccutchen-cci self-assigned this Dec 7, 2020
@kyleheadley
Copy link
Member Author

@mattmccutchen-cci The automation sounds great and I encourage you to start on that, either directly or through another issue. I think that with older tests or issues in active development, the automation will be complex enough that manual cleanup is more efficient where relevant. And as I suggested in the final sentence of the OP, this issue may never be complete, it just sets the current standard while we transition to automation as much as possible. Fee free to rename it to better distinguish it from the discussion of automation.

And while I'm thinking of it, @sroy4899 were you present during initial discussions? Do you have any suggestions? I think you worked with the testing more recently and thoroughly than the rest of us.

@mattmccutchen-cci
Copy link
Member

mattmccutchen-cci commented Dec 8, 2020

I think that with older tests or issues in active development, the automation will be complex enough that manual cleanup is more efficient where relevant.

I disagree, but he who gets to the work first decides how to do it. :/

And as I suggested in the final sentence of the OP, this issue may never be complete, it just sets the current standard while we transition to automation as much as possible.

ISTM this isn't an argument against going straight to automation for the bulk of the tests, where we can.

@mattmccutchen-cci mattmccutchen-cci changed the title Clean up tests Clean up test RUN commands Dec 9, 2020
@mattmccutchen-cci
Copy link
Member

Recording for future reference something I learned while working on #365: -verify is only accepted by clang -cc1, not by clang, so when we replace %clang_cc1 with %clang, we'll have to replace -verify with -Xclang -verify.

mattmccutchen-cci added a commit that referenced this issue May 11, 2021
headers.

%clang_cc1 expands to a command line containing -nostdsysteminc, which
turns off the use of the system headers.

Hope this doesn't affect the validity of the tests. We'll probably end
up doing this to all the 3C regression tests as part of #346, but for
now, minimize the number of tests we have to think about.
mattmccutchen-cci added a commit that referenced this issue May 18, 2021
headers.

%clang_cc1 expands to a command line containing -nostdsysteminc, which
turns off the use of the system headers.

Hope this doesn't affect the validity of the tests. We'll probably end
up doing this to all the 3C regression tests as part of #346, but for
now, minimize the number of tests we have to think about.
mattmccutchen-cci added a commit that referenced this issue May 20, 2021
This requires replacing `%clang_cc1 -verify` with `%clang -Xclang
-verify` in the tests that use it.

This is one part of the RUN command cleanup that we've wanted to do for
a while in #346, and now it is blocking part of #576. This is because
the single behavior difference we know of between %clang_cc1 and %clang
(other than the need for `-Xclang`) is that %clang_cc1 turns off the
system headers, and we want two of our tests that currently use
%clang_cc1 to be able to get checked declarations from the system
headers.

As recently as dac3f97 (August 2020),
all of the tests that used %clang_cc1 used -verify. Since then, a few
tests have been added that use %clang_cc1 and not -verify. This lends
support to my theory that the only reason we used %clang_cc1 was that we
were unaware that -verify could be used with %clang via -Xclang, and
some subsequently added tests copied the use of %clang_cc1 only because
the authors didn't know any better. Thus, I believe that the risk of the
migration affecting the validity of the tests (e.g., causing them to
falsely pass) is low.
mattmccutchen-cci added a commit that referenced this issue May 21, 2021
This requires replacing `%clang_cc1 -verify` with `%clang -Xclang
-verify` in the tests that use it.

This is one part of the RUN command cleanup that we've wanted to do for
a while in #346, and now it is blocking part of #576. This is because
the single behavior difference we know of between %clang_cc1 and %clang
(other than the need for `-Xclang`) is that %clang_cc1 turns off the
system headers, and we want two of our tests that currently use
%clang_cc1 to be able to get checked declarations from the system
headers.

As recently as dac3f97 (August 2020),
all of the tests that used %clang_cc1 used -verify. Since then, a few
tests have been added that use %clang_cc1 and not -verify. This lends
support to my theory that the only reason we used %clang_cc1 was that we
were unaware that -verify could be used with %clang via -Xclang, and
some subsequently added tests copied the use of %clang_cc1 only because
the authors didn't know any better. Thus, I believe that the risk of the
migration affecting the validity of the tests (e.g., causing them to
falsely pass) is low.
mattmccutchen-cci added a commit that referenced this issue Jun 21, 2021
Instead, put `FOO` on a separate run line. `cd` affects later RUN lines,
and `cd DIR && FOO` could mislead the reader that it doesn't and that's
why we needed to put both parts on one line. I added the proposed
guideline to #346.

I'm changing the existing tests before changing more tests to use `cd`
as part of this PR.
mattmccutchen-cci added a commit that referenced this issue Jun 21, 2021
Instead, put `FOO` on a separate run line. `cd` affects later RUN lines,
and `cd DIR && FOO` could mislead the reader that it doesn't and that's
why we needed to put both parts on one line. I added the proposed
guideline to #346.

I'm changing the existing tests before changing more tests to use `cd`
as part of this PR.
mattmccutchen-cci added a commit that referenced this issue Jun 22, 2021
…rectory. (#624)

* Follow new guideline: Avoid `cd DIR && FOO` in RUN lines.

Instead, put `FOO` on a separate run line. `cd` affects later RUN lines,
and `cd DIR && FOO` could mislead the reader that it doesn't and that's
why we needed to put both parts on one line. I added the proposed
guideline to #346.

I'm changing the existing tests before changing more tests to use `cd`
as part of this PR.

* Add `-o /dev/null` to clang commands that use `-` as input.

* Add -working-directory to clang commands with named input files.

* `cd` to a temporary directory for `3c -dump-stats` commands.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants