Test and fix absolute paths to compilation unit

[Flamefire]

• Rebase on Also add include in same directory #276

This adds an easy method to test all files also with absolute paths. Currently there are some issues when not using paths relative to the invocation directory. With the new tests this is caught.

The commits after that fix that behaviour so it works correctly.

[kimgr]

I'm not so sure about this, it makes little sense to me to run the same tests twice.

I'd prefer it if we locked in the absolute vs relative vs explicit vs implicit include path in focused tests (as you've done in #276). Or does this PR solve some additional problem?

[Flamefire]

Why doesn't it make sense, to check that the output of IWYU is the same for all testcases independent of whether the file path is given absolute or relative? IMO this is a very valid requirement. And why should we write additional tests that are only used for absolute paths? It would only result in duplicated test code, so this is the more elegant solution: Simply requiring the same output for relative and absolute paths for all the complicated cases that someone already invented.
I consider this a really important test part.

And yes 91f1243 and c9c2a3d do fix bugs occurring when using absolute or (different) relative paths

[kimgr]

For me, it's mostly about increased test runtime, they already take enough time to run that they slow me down when developing. Doubling that time does not seem desirable to me.

I haven't understood the details of the failures yet, but it appears to me that there are two test cases missing and you're effectively adding 93 new test cases. That seems wasteful to me. I don't need the test suite to find new bugs (though I think it's cool that you could exploit it that way), I need it to make sure the behavior we care about is preserved.

Let's hear if there are other thoughts, though.

[vsapsai]

For local development I don't want to run twice as many tests. But it would be useful to run them on buildbot. Does it make sense to run extended test suite based on some command line switch?

And it would be nice to have extensible mechanism for test variations. For example, I thought about running the same test with different levels of -std. It's not a requirement but it can help with avoiding too narrow naming and implementation.

[Flamefire]

Then I'd suggest a switch that toggles between absolute and relative paths. Additionally we should specify tests, that should be run in either setting. For example the check-also test which failed before this PR. This way you can run both for full test, one for development and still catch potential regressions due to the always run tests.

[Flamefire]

I rebased this PR and added 3 TestSuites: The DefaultSuite contains the current tests and the 4 failing tests w/o this PR, and a Relative/AbsolutePathSuite that contains all tests using relative or absolute paths respectively
This way one can run all tests with absolute path, with relative path or the default ones.

[kimgr]

I'm still not sold on the test variations, but I'd like to get the bugfix in place.

Can you pull bugfix + associated test out of this PR and into a new one?

[Flamefire]

What is wrong with the tests? I modified them as I understood @vsapsai wants them

For local development I don't want to run twice as many tests. But it would be useful to run them on buildbot. Does it make sense to run extended test suite based on some command line switch?

This is, what it is now: You can run the current tests without any additional param, all tests with absolute or relative paths with a special param and also single tests either absolute or relative. Per default only the ones, that failed before are included in execution.

I can/would not split this PR because you either have failing tests or a fix without tests. I consider both bad.

[kimgr]

Oh, I think I see now... The bug is only reproducible when absolute paths are passed to IWYU, and the test runner currently only supports relative paths?

With your patch applied I see 4 new failures (beyond badinc.cc which is already broken on my platform) -- is that expected?

Let me think this over for a bit, I think we can make this simpler and more explicit in the test runner.

[Flamefire]

This is correct.
One of the expected failures is the check-also feature. Another one is expected when assigning an associated header to the main unit. I'm actually surprised, that not more tests fail due to the 2nd issue. But I didn't bother trying to find the exact reason and just added those 4 failing tests to the default suite to avoid regressions after this.

I don't see how this can be made simpler than it is: Run default suite, absolute or relative paths or individual tests just by adding a command line param. The feature to run all tests with absolute paths can come handy, when similar bugs are tested, because IWYU should work, no matter how the path is passed. And absolute and relative path are the 2 corner cases for this. So if both of them work, everything should work.

[kimgr]

@Flamefire I'd still prefer to avoid the double-run thing proposed here. Have you had a chance to test latest IWYU -- there were a fair bit of relative-path fixes that came in recently, and I wonder if they have fixed whatever original problem you were seeing?

[Flamefire]

I'd still prefer to avoid the double-run thing proposed here.

This would only be done if explicitly requested and for a few existing tests which would currently fail IIRC. What is the exact issue you see? How would you do it?
I wanted to avoid code duplication, provide a way to run the tests with absolute or relative paths and a fast default test suite which only runs the necessary tests twice (abs/rel) in order to avoid regressions.

Have you had a chance to test latest IWYU

Unfortunately not. I can rebase this and test without the fix to verify. But need a while as I'm just back from vacation and quite busy the next couple days.

[kimgr]

There's already a lot of implicit stuff going on with the test runner (i.e. the framework has hard-coded IWYU/Clang switches for individual tests, and we need to state in comments what switches are assumed by the test). I'd just like to avoid more magic. I would prefer to use something like LLVM's test framework lit, where tests are marked up with explicit // RUN lines to drive the tool. That way it's clearly visible that a test is executed with a set of different args, and how that affects the outcome.

I've had a few scratch implementations of this going, but never finished anything. It would be nice to leverage lit directly, but it requires some bootstrapping to get going, and I haven't figured out how I want to make that work across all platforms.

No rush, but it would be great to hear how the current IWYU pans out. Thanks!

[Flamefire]

What would be considered the "current IWYU"? The master branch? Or the clang_8 branch?

I'd like to target the master (as in this PR) but failed to build it against the official bionic sources as first it failed for lit-cpuid (patched it out as in .travis.yml) and then it can't find the type FileEntryRef. Where would I find that?

Edit: Seems like that is part of Clang 10.x not of Clang 9.x: https://github.com/llvm/llvm-project/blob/release/9.x/clang/include/clang/Basic/FileManager.h

[kimgr]

Right, IWYU master follows Clang trunk. And the Debian packaging for Clang is broken, so you need to patch it as in .travis.yml it to use the CMake modules.

[Flamefire]

So what should this PR be against? Master? Then I'd need to build clang/llvm myself :/
Why is there no clang_9 branch as clang_x for lower versions?

I'm hesitant to build against an unreleased, just started version of clang/llvm instead of the current stable. Are fixes to IWYU backported to the other branches? Would it make sense to build this PR against clang_8 (or 9)? Are the path-fixes you mentioned part of the other branches?

[kimgr]

So many questions :)

So what should this PR be against? Master? Then I'd need to build clang/llvm myself :/

PRs should always be against master.

Why is there no clang_9 branch as clang_x for lower versions?

I haven't had time to release IWYU for clang 9 yet.

I'm hesitant to build against an unreleased, just started version of clang/llvm instead
of the current stable. Are fixes to IWYU backported to the other branches?

No, we develop on master and drop releases to be compatible with Clang releases (hence the clang_x branches). We don't backport, we barely have enough time to keep up with master.

Would it make sense to build this PR against clang_8 (or 9)?

No, I don't think so. It will never be released on those branches.

Are the path-fixes you mentioned part of the other branches?

Only in master, but should be part of the upcoming release for Clang 9.

Ideally, if Clang's Debian packaging wasn't broken, you should be able to just install
- llvm-10-dev
- llvm-10-tools
- libclang-10-dev
- clang-10

and use them for lib dependencies, but due to this upstream bug: https://bugs.llvm.org/show_bug.cgi?id=42432, it doesn't work out of the box.

[Flamefire]

Only in master, but should be part of the upcoming release for Clang 9.

But current master doesn't work for clang 9. :/

I'll check if there are packages for clang 10 already tomorrow

[kimgr]

That's correct, because Clang trunk has breaking changes compared to Clang 9. I will be branching clang_9.0 from before those changes were addressed on IWYU master.

I don't know what distro you're on, but the Debian packages on https://apt.llvm.org are definitely up to version 10 by now. See my work-in-progress MR for steps/patches to get it working: #719

[Flamefire]

I tried with the rebased version. If I remove the patch "Convert includee and main path to absolute path" then I get 3 failures: badinc_absolute, redecls_absolute, no_h_includes_cc_absolute. When removing "Store glob paths absolute and match against absolute paths" I get 1 failure: check_also_absolute

[kimgr]

Thanks! Then we have more work to do.

[Flamefire]

Without the ability to run tests with relative and absolute paths such failures won't be detected. I therefore strongly recommend to add the tests I created here. I'm happy to work with you to implement improvements, but IMO the current version is pretty solid.

On the test runtime issue: It only adds 4 more tests to the standard run_iwyu_test.py invocation and those are pretty fast. So I don't see an issue with having those as they show current issues and hence avoid regressions. It might be possible to keep only 1 of the 3 tests (badinc_absolute, redecls_absolute, no_h_includes_cc_absolute), but judging from the names I'd keep them as they might test issues in different areas.