This is an archive of the discontinued LLVM Phabricator instance.

[lit] Use path as test suite name
AbandonedPublic

Authored by modocache on Oct 8 2016, 11:47 AM.

Details

Summary

This resolves an item from utils/lit/TODO, which suggested that the
path to a test suite be used in lit output, instead of the test suite
name specified in the lit config. This makes re-running lit test
suites easier, because users are able to copy-and-paste the test suite
name.

Changing the lit output for LLVM may have unintended consequences, so
to stage the rollout of this feature, add an option to use the old
behavior, --use-test-suite-names. (Swift and other projects that use
LLVM will use the new behavior by default once they update their LLVM
checkouts).

I've tested these changes by running the utils/lit/tests (all passing),
as well as check-llvm with the option enabled and disabled.

Event Timeline

modocache updated this revision to Diff 74039.Oct 8 2016, 11:47 AM
modocache retitled this revision from to [lit] Use path as test suite name.
modocache updated this object.
modocache added a subscriber: llvm-commits.
modocache added inline comments.Oct 8 2016, 11:51 AM
utils/lit/TODO
44 ↗(On Diff #74039)

@ddunbar @delcypher FYI I'm planning on submitting LLVM PRs for each item in this TODO file this weekend, then submitting a diff to delete it.

modocache updated this revision to Diff 74041.Oct 8 2016, 11:59 AM

Update lit documentation to reflect name no longer necessary.

modocache added inline comments.Oct 13 2016, 12:46 PM
utils/lit/TODO
44 ↗(On Diff #74039)

Submitted a patch for this, in D25496.

kastiglione added inline comments.
utils/lit/tests/discovery.py
14

Can %s or some substitution be used to avoid repetition, and make file renaming more straight forward?

modocache updated this revision to Diff 75532.Oct 22 2016, 11:33 AM

Rebased onto recent lit changes.

modocache added inline comments.Oct 22 2016, 11:42 AM
utils/lit/tests/discovery.py
14

Good suggestion, but I believe that substitutions are only available for lit RUN commands, not for input to FileCheck. I might be wrong, but I don't think we'll be able to use something like %{inputs}/discovery here.

@delcypher @ddunbar Friendly ping! Let me know if this diff is too big, I could try to split it up. The majority of the changes are to lit's own test suite.

delcypher edited edge metadata.Nov 3 2016, 11:42 AM

Sorry for taking so long to look at this. I've taken a quick glance over the code changes. They look reasonable to me. I'm not sure about the output of lit though (see my other comments).

utils/lit/tests/discovery.py
14

@modocache I believe you are correct that the substitutions are only available in RUN: lines. You could use FileCheck's variable support (http://llvm.org/docs/CommandGuide/FileCheck.html#filecheck-variables ) to remove some duplication if you wanted to but I prefer the clarity of how the test is written now.

22–26

I'm not sure if this is the output that @ddunbar intended. One of the motivations for making this change is so that paths to tests could be copied and pasted and given to lit. If I'm reading these test correctly there's :: in between the root path to the test and path to the test in the test suite.

utils/lit/tests/xunit-output.py
8

You should probably talk to some of the consumers of this format about this change. A quick git blame suggestes that talking to Chris Matthews and David Chisnall about this would be a good place to start.

modocache planned changes to this revision.Nov 3 2016, 4:39 PM
modocache added subscribers: cmatthews, theraven.

Thanks for the review, @delcypher! I'm going to remove the extraneous :: and update the tests.

utils/lit/tests/discovery.py
22–26

Exactly right! I spoke with @ddunbar today and indeed he suggested I remove this :: . Patch incoming.

utils/lit/tests/xunit-output.py
8

Great idea, thanks! Chris, David, this diff changes lit to output the absolute path to each test file, instead of TestSuiteName :: relative/path/to/test. As a result, this xunit format will become slightly longer. Is this alright?

/cc @cmatthews @theraven

modocache updated this revision to Diff 76938.Nov 4 2016, 1:52 PM
modocache edited edge metadata.

Don't use double colon for test suites using the absolute path instead of test names.

@modocache Thanks for updating.

ping: @cmathews @theraven

It would be a good idea to test this one with a Jenkins instance. The xunit reports were designed to split the test names in a way that seemed natural in the Jenkins test reports.

For example:

http://lab.llvm.org:8080/green/job/clang-stage1-configure-RA_check/24377/testReport/

The test-suite name is mapped into the Package name, all the intermediate folder names are the class name, and the file name gets mapped to the test name. This is to keep the tree to 3 levels.

It may be the case that the buildbots parse these names from the process output, not the xunit file. For example in zorg's LitTestCommand, I believe it is looking for the :: to generate the buildbot test results. Galina might know more about that.

gkistanova added a subscriber: gkistanova.EditedDec 6 2016, 11:35 AM

LitTestCommand parses LIT printout, looking for the ::, then uses the built "path" to make temp files with log chunks.

I have done a change to skip some intermediate folder names, as it was running out of the MAX_PATH on bots. If you want to have the absolute path there, we should have a reliable way to tell the root then, which could be stripped for these purposes. Ideas?

If you want to change the printout, we would need to change the LitTestCommand in advance, otherwise you would get a lot of bot failures.

I'll see if I can change the LitTestCommand to support both formats during the transition. Unless this is something you plan to do yourself.

modocache abandoned this revision.Nov 21 2019, 8:13 PM

Sorry for the noise bringing up this change, I don't think I'll be pursuing this after all.