Make testActiveDocumentsGetBestPriority() clean up after itself
ClosedPublic

Authored by thomassc on Jan 27 2019, 7:15 PM.

Details

Summary

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).

Test Plan

Run plugins/clang/tests/test_duchain-clang

Diff Detail

Repository
R32 KDevelop
Branch
arcpatch-D18567
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 10461
Build 10479: arc lint + arc unit
thomassc created this revision.Jan 27 2019, 7:15 PM
Restricted Application added a project: KDevelop. · View Herald TranscriptJan 27 2019, 7:15 PM
Restricted Application added a subscriber: kdevelop-devel. · View Herald Transcript
thomassc requested review of this revision.Jan 27 2019, 7:15 PM
mwolff requested changes to this revision.Jan 27 2019, 7:21 PM
mwolff added a subscriber: mwolff.

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

This revision now requires changes to proceed.Jan 27 2019, 7:21 PM
thomassc updated this revision to Diff 51637.Feb 14 2019, 12:55 AM

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?

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.

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.

thomassc updated this revision to Diff 52405.Feb 23 2019, 8:53 PM

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.

mwolff requested changes to this revision.Mar 10 2019, 7:01 PM

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!

This revision now requires changes to proceed.Mar 10 2019, 7:01 PM
arrowd commandeered this revision.Apr 5 2019, 12:56 PM
arrowd added a reviewer: thomassc.
arrowd updated this revision to Diff 55469.Apr 5 2019, 12:56 PM

Simplify code.

test_duichain-clang passes.

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.

thomassc commandeered this revision.Apr 6 2019, 2:56 PM
thomassc updated this revision to Diff 55566.
thomassc edited reviewers, added: arrowd; removed: thomassc.

Thanks for the info. I didn't spot it at first since it is hidden in a dropdown menu.

mwolff accepted this revision.Apr 13 2019, 4:41 PM

thanks, lgtm!

This revision is now accepted and ready to land.Apr 13 2019, 4:41 PM
This revision was automatically updated to reflect the committed changes.