[PATCH] D34132: Add scripts to perform LNT submissions

Matthias Braun via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Mon Jun 19 11:47:21 PDT 2017


MatzeB added a comment.

In https://reviews.llvm.org/D34132#784019, @kristof.beyls wrote:

> There is quite a bit of code that is duplicated from LNT to test-suite as part of this patch.
>  I'm not entirely sure if having this much code duplication between LNT and test-suite is desirable, unless this is part of a bigger redesign, like we discussed in January in the thread starting roughly at http://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20170123/423073.html, with a nicer design in the end?




1. Yes this breaks the dependency on LNT for the test running and data submission as discussed. Doesn't feel like a "bigger redesign" to me though as this patch is mostly it.
2. I will resolve the duplication between `utils/lntsubmit` and `lnt/tests/test_suite.py` in a followup by making the latter use the former.
3. The duplication between `utils/compilerinfo` and `lnt/testing/util/compilers.py` is trickier as we need to keep the code in LNT for the `runtest nts` and `runtest compile` modes. At the same time the point here is to break dependencies to LNT so we shouldn't use LNTs code. That leaves us those possibilities:
  - The radical solution: We stop adding all the compilerinfo to each submission. I'm not gonna start that discussion here :)
  - Not copying the code but writing a new version of it. But what's the point...
  - I actually have a commit that refactors LNTs compilerinfo file to look exactly like the one here by moving `lnt/testing/util/compilers.py` to a new `lnt/tools/compilerinfo.py` and adding the json printing bits when it is run as a standalone tool while the other LNT code can keep importing it. At that point we could keep LNT and test-suite in sync by copying that file over. It just felt slightly unmotivated/out of place on the LNT side. But I can submit that patch again if you prefer the way it makes it easier to keep the two in sync.
  - We are only talking about 300 lines of code here. Yes code duplication is bad, but we have to weigh that against the costs of having LNT as a dependency for running the test-suite. (Or if we would choose to do so the cost of factoring the common functionality out into a shared library).

> As I said before, I like the conceptual idea of having the functionality to drive the test-suite being part of the "test-suite" project rather than the "LNT" project, but I haven't thought further about practical consequences, like potential code duplication, showing up here.
>  My gut feel is that most of these issues could go away in the end if we'd put all code to drive the test-suite in test-suite, and make LNT "just" the web server/database/analysis functionality.
>  But it's probably best to make sure we agree that's where we want to end up before adding a lot of code duplication as part of this first step patch?

As a data point: We already have at least 1 internal benchmark that works by uploading the data directly to the `submitRun` URL on the jenkins server instead of going through `lnt runtest`. And as far as I know that works fine (though I'm not directly involved in setting this up/maintaining it).

> Apart from my other thoughts above, I think this is the point where it is useful to start adding some documentation on how to run the tests for the test-suite framework, e.g. similar in spirit to http://lnt.readthedocs.io/en/latest/developer_guide.html.

Yes I can do that.


Repository:
  rL LLVM

https://reviews.llvm.org/D34132





More information about the llvm-commits mailing list