search

HaveF / include-what-you-use

Automatically exported from code.google.com/p/include-what-you-use
Other
0 stars 0 forks source link

Summary diffs always fail when running the tests on Windows #53

Closed GoogleCodeExporter closed 9 years ago

GoogleCodeExporter commented 9 years ago 
When running the tests on Windows, _CompareExpectedAndActualSummaries always 
generates failures as it detects differences in the paths embedded in the 
summary:

Unexpected summary diffs for tests\system_namespaces.cc:
+++
@@ -1,10 +1,10 @@
-tests/system_namespaces.cc should add these lines:
+tests\system_namespaces.cc should add these lines:
 namespace notsys_ns { template <typename T> class TplClass; }

-tests/system_namespaces.cc should remove these lines:
+tests\system_namespaces.cc should remove these lines:
 - #include "tests/system_namespaces-d3.h"  // lines XX-XX

-The full include-list for tests/system_namespaces.cc:
+The full include-list for tests\system_namespaces.cc:
 #include "tests/system_namespaces-d1.h"  // for StdClass
 #include "tests/system_namespaces-d2.h"  // for SystemClass
 namespace notsys_ns { template <typename T> class TplClass; }

---

I had thought to make PrintableDiffs canonicalise 'filename' on entry, but as 
the path is preserved as specified on the command line, it's possible to fix 
this entirely within iwyu_test_util.py.

A simpler fix is to do the following fixup on entry to TestIwyuOnRelativeFile:

cc_file = cc_file.replace("\\","/")
cpp_files_to_check = [file.replace("\\","/") for file in cpp_files_to_check]

I'm assuming that these calls to replace() will no-op on Linux, but I could 
guard that code with 'if sys.platform == 'win32':' if that would be preferable.

Original issue reported on code.google.com by paul.hol...@gmail.com on 16 Jul 2011 at 5:50

Attachments:

GoogleCodeExporter commented 9 years ago 
Looks good to me.  Feel free to commit.

One style nit: should be space after the comma in "\\","/" (two places).

Original comment by csilv...@gmail.com on 18 Jul 2011 at 11:55

GoogleCodeExporter commented 9 years ago 
Hmm, now that I think of it, depending on how you fix issue 54 this may not 
need to be changed at all.  Let's hold off on this until issue 54 is resolved.

Original comment by csilv...@gmail.com on 19 Jul 2011 at 12:02

GoogleCodeExporter commented 9 years ago 
I think in this case there will still need to be some fixup in the python code.

The os.path.join that is used in _GetAllCppFilesUnderDir to generate 
cpp_files_to_check ends up inserting backslashes on Windows.

Now that IWYU is canonicalising paths (Issue 54, Issue 55), the summary text 
that is generated contains forward slashes.

This means that _GetExpectedSummaries ends up creating a dictionary with keys 
containing backslashes, and _GetActualSummaries ends up creating a dictionary 
with keys containing forward slashes. When _CompareExpectedAndActualSummaries 
comes to compare the dictionaries, it fails to match up the summaries.

There's a couple of different approaches that would work:

(1) canonicalise cpp_files_to_check (the patch above, but cc_file no longer 
needs to be changed)
(2) canonicalise paths before adding entries to the dictionaries in 
_GetExpectedSummaries and _CompareExpectedAndActualSummaries (probably just 
needs doing in _GetExpectedSummaries)

There's possibly an even simpler solution which I've overlooked?

Original comment by paul.hol...@gmail.com on 19 Jul 2011 at 10:34

GoogleCodeExporter commented 9 years ago 
} There's possibly an even simpler solution which I've overlooked?

How about using '/'.join() instead of os.path.join(), with a comment that iwyu 
only uses / internally?

But maybe that won't work great because the inputs to os.path.join could 
already have '/' in them.  Maybe you could do something like 'path = 
path.replace(os.sep, '/')?

I think, again, we want to make the substitution as close to the 'input' to 
iwyu as we can.  So I'd prefer something that normalizes sooner, rather than in 
cpp_files_to_check (which is pretty late).

Original comment by csilv...@gmail.com on 19 Jul 2011 at 11:16

GoogleCodeExporter commented 9 years ago 
It turns out that this canonicalisation can be done as early as 
run_iwyu_test.py/OneIwyuTest/RunOneTest which I do in the attached patch. I 
tried moving this a little earlier still - canonicalising filename rather than 
files_to_check - but it seems that the glob.glob inserts backslashes on Windows.

How does this look?

Incidentally, I had been assuming that the tests were being invoked via 
iwyu_test_util.py/TestIwyuOnFile, which calls _GetAllCppFilesUnderDir. I can't 
see where this might be called from. The comment for TestIwyuOnFile indicates 
that the function might no longer be needed. Is that the case?

Original comment by paul.hol...@gmail.com on 20 Jul 2011 at 9:31

Attachments:

GoogleCodeExporter commented 9 years ago 
run_iwyu_tests.py calls TestIwyuOnRelativeFile.  It doesn't call 
_GetAllCppFilesUnderDir(), as you noticed, and instead just does globs itself.

Patch looks good to me -- that's where I would make this canonicalization call 
too.  Feel free to commit!

Original comment by csilv...@gmail.com on 20 Jul 2011 at 11:09

GoogleCodeExporter commented 9 years ago 
Great. That's applied in r283, and resolves this issue.

Original comment by paul.hol...@gmail.com on 21 Jul 2011 at 7:12