| kossebau |
Core::self()->documentController()->documentForUrl(url) can return a nullptr and apparently does so reliably when running the plugins/clang/tests/test_assistants test on Mac.
This patch prevents crashing when that happens, but is it the most appropriate fix?
without the patch: crash (on Mac)
with the patch: no crash and test results are identical to the ones on Linux.
| Lint Skipped |
| Unit Tests Skipped |
| Buildable 1191 | |
| Build 1204: arc lint + arc unit |
This patch prevents crashing when that happens, but is it the most appropriate fix?
What is your own answer to this? ;)
Surely there must be something wrong in the internal logic, given that TestBed class seems to try to wrap an instance of a document. Please try to investigate why the document is not returned from the controller as one would expect by the code.
Sadly for me locally the tests fails for other reasons, so I cannot give you some results to compare for. CI at least for stable on openSUSE also does not show crashes, only other issues.
This patch just puts a cover over any potential issues, not good. We should know whether to expect from the documentcontroller to return a document for the given url. Please consider to help by investigating this more deeply.
What is your own answer to this? ;)
Would I ask the question if I had the answer?
Given that this issue doesn't occur in KDevelop itself (nor in the test on Linux) I'd say one would either handle the error situation gracefully, or bail out from the test as soon as it's detected.
Please try to investigate why the document is not returned from the controller as one would expect by the code.
On Linux, Core::self()->documentController()->openDocument(url) returns the same pointer as Core::self()->documentController()->documentForUrl(url) does later. On Mac the former returns a valid pointer, the latter returns NULL. It looks like this may well mean that the document isn't actually opened (or not registered in DocumentCOntrollerPrivate::documents but that seems extremely unlikely).
Edit: this may be it:
DocumentControllerPrivate::openDocumentInternal is one of those functions that uses asserts instead of proper runtime checks that don't disappear in release builds:
// The url in the document must equal the current url, else the housekeeping will get broken Q_ASSERT(!doc || doc->url() == url);
adding that comparison and trace output to test_assistants.cpp gives this on the terminal:
QWARN : TestAssistants::testRenameAssistant(Prepend Text) openDocument QUrl("file:///var/folders/j1/1439ppj08xj8h6006s6drbq00000gs/T/test_assistants-OT8CTF/0.h") doc= 0x7fe1532bd6a0
QSYSTEM: TestAssistants::testRenameAssistant(Prepend Text) doc->url= QUrl("file:///private/var/folders/j1/1439ppj08xj8h6006s6drbq00000gs/T/test_assistants-OT8CTF/0.h") != QUrl("file:///var/folders/j1/1439ppj08xj8h6006s6drbq00000gs/T/test_assistants-OT8CTF/0.h")
QWARN : TestAssistants::testRenameAssistant(Prepend Text) docForUrl QUrl("file:///var/folders/j1/1439ppj08xj8h6006s6drbq00000gs/T/test_assistants-OT8CTF/0.h") (adjusted= QUrl("file:///var/folders/j1/1439ppj08xj8h6006s6drbq00000gs/T/test_assistants-OT8CTF/0.h") )= 0x0This is indeed a Mac-specific issue: /var is a symlink to /private/var and file name normalisation is almost unavoidable on Mac (and hardcoded here, I think).
That means the situation is to be expected and my fix isn't wrong in itself, but maybe we can avoid the cause by doing the normalisation step after generating the url and before using it(?).
This should be a real fix for the issue I've been seeing.
I've kept a single nullptr check in order to print a warning, and allow the test to crash further on.
This is an even lower-level fix which applies to all cases where the temporary directory is obtained by reading TMPDIR. That variable contains the path containing the /var symlink; the patch basically sets the variable to the canonical representation of itself, or else sets it to the fallback temp. dir.