Page MenuHomePhabricator

Add initial tests for update_{llc,cc}_test_checks.py
ClosedPublic

Authored by arichardson on Nov 25 2019, 2:08 AM.

Details

Summary

This commit adds two basic tests for these update script to validate that
they still work as expected. In the future we could extend these tests
whenever new features are added to avoid introducing regressions.

I am not sure this is the best approach for testing, but it did allow me to find a python2.7 compat issue that I introduced in D70432.
If this approach seems fine, I'll also add a basic test for the remaining update scripts.

Diff Detail

Event Timeline

arichardson created this revision.Nov 25 2019, 2:08 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 25 2019, 2:09 AM

Happy to see this! We should definitelly have such test suite.

Thanks for working on this.

Two general questions, but it generally looks fine to me:

Why do we have the extra input folders that contain all files?
Could you add the same for the opt script?

llvm/test/tools/UpdateTestChecks/update_cc_tests/mangled_names.test
31 ↗(On Diff #230856)

Are these (EMPTY lines) auto generated?

maybe diffing with reference output file is enough?

checks for checks are kinda weird imho.

MaskRay added inline comments.Nov 26 2019, 10:36 AM
llvm/test/tools/UpdateTestChecks/lit.local.cfg
19

Use " when nested ' is used to reduce escaping.

llvm/test/tools/UpdateTestChecks/update_cc_tests/Inputs/mangled_names.c
12 ↗(On Diff #230856)

Remove trailing empty lines.

llvm/test/tools/UpdateTestChecks/update_cc_tests/mangled_names.test
54 ↗(On Diff #230856)

Remove trailing empty line.

llvm/test/tools/UpdateTestChecks/update_llc_tests/basic.test
4 ↗(On Diff #230856)

Delete -f. It is not needed.

I get another idea. We can probably use sed to delete # lines in %s and feed that to %update_llc_test_checks.

arichardson marked 4 inline comments as done.

Address comments

Use diff -u against an expected output instead of lots of CHECK-LINES with awkward escaping.
Since we are using the lit internal shell this should work everywhere.

llvm/test/tools/UpdateTestChecks/update_cc_tests/mangled_names.test
31 ↗(On Diff #230856)

No, I added them manually. All the regex escaping is quite awkward so I've now gone with the diff suggestion.

arichardson marked an inline comment as done.
arichardson added inline comments.
llvm/test/tools/UpdateTestChecks/update_cc_test_checks/mangled_names.test
15

Note: this check fails without D70890

I have two minor comments, neither is blocking. @MaskRay @xbolva00, please review and accept without waiting for me, I'm fine with this.

  • We still have the "inputs" folder in which everything lives. I could see the '.ll' and '.ll.expected' to live in their but why the tests?
  • The --function-signatures test has manual check lines. We should probably move to a '.expected' file as well here.
llvm/test/tools/UpdateTestChecks/update_test_checks/basic.test
45

Shouldn't we have multiple .expected files instead? I see myself struggle with the escaping already.

Address comments

Use diff -u against an expected output instead of lots of CHECK-LINES with awkward escaping.
Since we are using the lit internal shell this should work everywhere.

Cool! Thanks.

arichardson marked an inline comment as done.Dec 2 2019, 10:29 AM

I have two minor comments, neither is blocking. @MaskRay @xbolva00, please review and accept without waiting for me, I'm fine with this.

  • We still have the "inputs" folder in which everything lives. I could see the '.ll' and '.ll.expected' to live in their but why the tests?

Only the .ll and .ll.expected is in the Inputs folder. The .test shouldn't be.
I could also move the .expected out of the inputs folder which would make some of the check lines simpler since I can just use diff -u %t.ll %s.expected then.

  • The --function-signatures test has manual check lines. We should probably move to a '.expected' file as well here.

I am happy to move to a separate .expected file for the --function-signatures test. What do you think @MaskRay @xbolva00 ?

llvm/test/tools/UpdateTestChecks/update_test_checks/basic.test
45

Since these files are quite small I guess that would be fine. I agree that the escaping is ugly.

The only benefit of having the manual check lines is that it shows the difference between the two files but that can also be seen by comparing the <test>.expected with a <test>.funcsig.expected

jdoerfert accepted this revision.Dec 8 2019, 10:11 PM

LGTM with two comments. (You can commit with the appropriate changes.)

  • We still have the "inputs" folder in which everything lives. I could see the '.ll' and '.ll.expected' to live in their but why the tests?

Only the .ll and .ll.expected is in the Inputs folder. The .test shouldn't be.

  1. I mean that is what I want but the current version looks different.

I am happy to move to a separate .expected

  1. Please do.
llvm/test/tools/UpdateTestChecks/update_test_checks/basic.test
45

I'd prefer a new file.

This revision is now accepted and ready to land.Dec 8 2019, 10:11 PM
MaskRay added inline comments.Dec 9 2019, 10:50 AM
llvm/test/tools/UpdateTestChecks/lit.local.cfg
7

What's the status of Python 3 compatibility of lit? Can we skip the tests if the legacy Python 2 is used?

49

'\'' -> "'"

Why isn't this shell_quote?

arichardson marked 2 inline comments as done.Dec 9 2019, 10:57 AM
arichardson added inline comments.
llvm/test/tools/UpdateTestChecks/lit.local.cfg
7

lit works with python3. However, the default is still to run lit with python2.7 and I would like these tests to always run rather than only for those who configure LLVM with DPYTHON_EXECUTABLE=/usr/bin/python3

49

Will fix, I added this line before realizing that I needed more things to be quoted.

arichardson marked 4 inline comments as done.
  • Use separate .expected files for the --function-signature tests
  • address comments
Closed by commit rG240aff80e0e5: Add initial tests for update_{llc_,cc_,}test_checks.py (authored by Alex Richardson <Alexander.Richardson@cl.cam.ac.uk>). · Explain WhyDec 16 2019, 3:43 AM
This revision was automatically updated to reflect the committed changes.
thakis added a subscriber: thakis.EditedDec 16 2019, 8:03 AM

Looks like the test fails on macOS: http://45.33.8.238/mac/4235/step_11.txt

^ Sorry, please ignore that part. I'm still looking if the failure isn't my fault with the bot setup. I meant to delete this after I wrote the py3 question.

llvm/test/tools/UpdateTestChecks/lit.local.cfg
49

Nothing in this patch seems to use this as far as I can tell? Do you need this?

arichardson marked an inline comment as done.Dec 16 2019, 8:09 AM
arichardson added inline comments.
llvm/test/tools/UpdateTestChecks/lit.local.cfg
49

Yes indeed it is not needed anymore. In an initial version of the patch I used %python3 to invoke update_cc_test_checks.py Does this cause issues for you?
If so feel free to delete.

thakis added inline comments.Dec 16 2019, 11:22 AM
llvm/test/tools/UpdateTestChecks/lit.local.cfg
49

I finally had time to look into why this is failing on one of my machines. If python3 isn't on the path, then the update_cc_test_checks substitution isn't getting added, and RUN: %update_cc_test_checks in the test tries to literally run %update_cc_test_checks (which of course fails).

So a possible fix is to add REQUIRES: python3to the test.

It seems a bit strange to me to only run some tests if py3 is around, though. Environment-dependent tests like this make analyzing failures more difficult. What stops these tests from running with Py3?

(ps: It'd be nice if add_update_script_substition would expect that the name already contains a leading % instead of adding it. That makes it much easier to grep for %update_cc_test_checks and see where it's supposed to be set.)

I added a REQUIRES: line (and did the greppability suggestion) in cc802ea67beb66. I'd be interested in ways to make these tests not depend on py3 though.

I added a REQUIRES: line (and did the greppability suggestion) in cc802ea67beb66. I'd be interested in ways to make these tests not depend on py3 though.

We should be able to use sys.executable instead if its python3.
I'm not sure how much longer python 2 will be used by default for running tests. Once that changes we can use sys.executable unconditionally.

rnk added a subscriber: rnk.Thu, Dec 26, 2:16 PM

I reverted the update_cc_* part of this patch in 287307a0c60b68099d5f9dd22ac1db2a42593533. Testing for python3 on PATH and assuming that it's going to work with no way for the user to disable the tests from cmake doesn't seem like a great approach. This essentially creates a test suite dependency on py3, and IIRC on llvm-dev we agreed we don't want that, we want to be able to check out, build, & test llvm without py3.

I would prefer that this reland as something that uses config.python_executable with a check for python 3 in cmake.