This patch makes testActiveDocumentsGetBestPriority() wait for the TestFiles it uses to be parsed, which prevents clang errors like "error: error reading '/tmp/testfile_SD1554.cpp'" that happen if these files get removed early. It also makes this test close the documents it opens, which prevents potential crashes in TestDUChain::cleanupTestCase() (and perhaps other issues).
Details
- Reviewers
mwolff arrowd - Group Reviewers
KDevelop - Commits
- R32:4acac3f79695: TestFile: On destruction, close associated document if open and stop the…
Run plugins/clang/tests/test_duchain-clang
Diff Detail
- Repository
- R32 KDevelop
- Lint
Automatic diff as part of commit; lint not applicable. - Unit
Automatic diff as part of commit; unit tests not applicable.
great initiative, but could you check if this could be done in a more generic way potentially? if not then I'm all for getting this in as-is otherwise
plugins/clang/tests/test_duchain.cpp | ||
---|---|---|
1004 ↗ | (On Diff #50389) | could we handle this generically in ~TestFile somehow? Maybe by revoking the background parser job there, or if it was started already, waiting for that? |
1008 ↗ | (On Diff #50389) | could you add a smartptr like handle for that which does this automatically on destruction? we do this in a few places, and it would be better if it's done via RAII |
Regarding waiting for the background parser on TestFile destruction, I didn't find a way to query whether a parse job is running or to wait for it, if it was started externally, using existing code (but I'm also not familiar with the codebase). As an alternative, one can do Q_ASSERT(ICore::self()->languageController()->backgroundParser()->isIdle()); in the test cleanup function to ensure that the tests don't leave the background parser running. This might also be a good idea to ensure that the tests don't influence each other in that way.
Regarding document handling, I'm not sure whether it's worth adding a specific test class for the few cases here. I found that in test_buddies.cpp, the test cleanup function does m_documentController->closeAllDocuments();. I tried this for the duchain test, but this caused a crash when this actually tried to close the open documents, which I wasn't motivated to debug. Alternatively, one can do Q_ASSERT(ICore::self()->documentController()->openDocuments().isEmpty()); similarly to the case above to ensure that the tests clean up after themselves. Would you be fine with that?
One idea: In ~TestFile do these things:
a) close and discard the document, if it's opened currently
b) BackgroundParser::removeDocument
c) if BackgroundParser::parseJobForDocument returns non-null, await the parseJobFinished signal for that url with some timeout. You can do that e.g. via:
if (backgroundParser->parseJobForDocument(url)) { bool finished = false; connect(backgroundParser, &BackgroundParser::parseJobFinished, this, [this, &finished](const IndexedString& finishedUrl) { if (finishedUrl == url) finished = true; }; QTRY_VERIFY(finished); }
Regarding document handling, I'm not sure whether it's worth adding a specific test class for the few cases here. I found that in test_buddies.cpp, the test cleanup function does m_documentController->closeAllDocuments();. I tried this for the duchain test, but this caused a crash when this actually tried to close the open documents, which I wasn't motivated to debug. Alternatively, one can do Q_ASSERT(ICore::self()->documentController()->openDocuments().isEmpty()); similarly to the case above to ensure that the tests clean up after themselves. Would you be fine with that?
Right, so we could just do that in ~TestFile maybe? Anyhow, ignore this for now if it's too much trouble.
Thanks for pointing me to the right places.
I didn't consider parseJobForDocument() before since its documentation comment says:
* This may not contain all of the parse jobs that are intended * unless you call in from your job's ThreadWeaver::Job::aboutToBeQueued() * function.
Note, while testing, according to debug output, backgroundParser->removeDocument() always removed the queued documents. The if (backgroundParser->parseJobForDocument(d->url)) case did not actually run in my tests.
cool, this is great! and it fixes the issue you originally found in testActiveDocumentsGetBestPriority?
kdevplatform/tests/testfile.cpp | ||
---|---|---|
128 | if (auto *document = ICore::self()->...) { document->close(...); } | |
134 | maybe replace this whole if with a simple QTRY_VERIFY(!backgroundParser->parseJobForDocument(d->url))); if I'm not mistaken, it will have the same effect as the multiple lines of code that you wrote! |
Thanks for the update. Seems good to me.
Yes, I think this should fix the original issue, since it manages to close the documents that the test leaves open without crashing, and when adding the new tests from the other patch that first caused the issue, I don't see any problems anymore.
I tried to also address Milian's other comment, but seemingly couldn't update the patch here anymore (or didn't find how to). So this would be the diff with use of the auto:
diff --git a/kdevplatform/tests/testfile.cpp b/kdevplatform/tests/testfile.cpp index aab8906f3a..29f4e572d7 100644 --- a/kdevplatform/tests/testfile.cpp +++ b/kdevplatform/tests/testfile.cpp @@ -30,6 +30,7 @@ #include <language/duchain/duchain.h> #include <language/backgroundparser/backgroundparser.h> #include <interfaces/icore.h> +#include <interfaces/idocumentcontroller.h> #include <interfaces/ilanguagecontroller.h> #include <project/projectmodel.h> @@ -123,6 +124,14 @@ TestFile::TestFile(const QString& contents, const QString& fileExtension, const TestFile::~TestFile() { + if (auto* document = ICore::self()->documentController()->documentForUrl(d->url.toUrl())) { + document->close(KDevelop::IDocument::Discard); + } + + auto backgroundParser = ICore::self()->languageController()->backgroundParser(); + backgroundParser->removeDocument(d->url, this); + QTRY_VERIFY(!backgroundParser->parseJobForDocument(d->url)); + if (d->topContext && !d->keepDUChainData) { DUChainWriteLocker lock; DUChain::self()->removeDocumentChain(d->topContext.data());
@thomassc it is because I commandeered your revision to be able to update it. If you want, you do the same. First select "Commandeer Revision" at the bottom of this page, and then use arc diff (possibly with --update D18567) to update it.
Thanks for the info. I didn't spot it at first since it is hidden in a dropdown menu.