Add unit test that checks Format data
ClosedPublic

Authored by dhaumann on Aug 21 2018, 6:38 PM.

Details

Summary

This unit test loads just one definition including its included defintions.
Then it gets all Formats of the definition + includedDefinitions(). In
theory, the format IDs now should be 1, 2, 3, ..., N.

What's currently not clear to me is whether this test is really correct.
I think so, but Jira.xml fails...

Test Plan

make && make test

Diff Detail

Repository
R216 Syntax Highlighting
Branch
check-full-formats (branched from master)
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 2092
Build 2110: arc lint + arc unit
dhaumann created this revision.Aug 21 2018, 6:38 PM
Restricted Application added projects: Kate, Frameworks. · View Herald TranscriptAug 21 2018, 6:38 PM
Restricted Application added subscribers: kde-frameworks-devel, kwrite-devel. · View Herald Transcript
dhaumann requested review of this revision.Aug 21 2018, 6:38 PM

contains 363 Normal Text Alerts_indent is missing.

What fails here is Jira.xml:

QDEBUG : KSyntaxHighlighting::RepositoryTest::testIncludedFormats() syntaxrepository_test(4584)/(default) ?[31m?[34mKSyntaxHighlighting::RepositoryTest::testIncludedFormats?[0m: QVector(1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25, 26, 27, 28, 29, 30, 31, 32, 33, 34, 35, 36, 37, 38, 39, 40, 41, 42, 43, 44, 45, 46, 47, 48, 49, 50, 51, 52, 53, 54, 55, 56, 57, 58, 59, 60, 61, 62, 63, 64, 65, 66, 67, 68, 69, 70, 71, 72, 73, 74, 75, 76, 77, 78, 79, 80, 81, 82, 83, 84, 85, 86, 87, 88, 89, 90, 91, 92, 93, 94, 95, 96, 97, 98, 99, 100, 101, 102, 103, 104, 105, 106, 107, 108, 109, 110, 111, 112, 113, 114, 115, 116, 117, 118, 119, 120, 121, 122, 123, 124, 125, 126, 127, 128, 129, 130, 131, 132, 133, 134, 135, 136, 137, 138, 139, 140, 141, 142, 143, 144, 145, 146, 147, 148, 149, 150, 151, 152, 153, 154, 155, 156, 157, 158, 159, 160, 161, 162, 163, 164, 165, 166, 167, 168, 169, 170, 171, 172, 173, 174, 175, 176, 177, 178, 179, 180, 181, 182, 183, 184, 185, 186, 187, 188, 189, 190, 191, 192, 193, 194, 195, 196, 197, 198, 199, 200, 201, 202, 203, 204, 205, 206, 207, 208, 209, 210, 211, 212, 213, 214, 215, 216, 217, 218, 219, 220, 221, 222, 223, 224, 225, 226, 227, 228, 229, 230, 231, 232, 233, 234, 235, 236, 237, 238, 239, 240, 241, 242, 243, 244, 245, 246, 247, 248, 249, 250, 251, 252, 253, 254, 255, 256, 257, 258, 259, 260, 261, 262, 263, 264, 265, 266, 267, 268, 269, 270, 271, 272, 273, 274, 275, 276, 277, 278, 279, 280, 281, 282, 283, 284, 285, 286, 287, 288, 289, 290, 291, 292, 293, 294, 295, 296, 297, 298, 299, 300, 301, 302, 303, 304, 305, 306, 307, 308, 309, 310, 311, 312, 313, 314, 315, 316, 317, 318, 319, 320, 321, 322, 323, 324, 325, 326, 327, 328, 329, 330, 331, 332, 333, 334, 335, 336, 337, 338, 339, 340, 341, 342, 343, 344, 345, 346, 347, 348, 349, 350, 351, 352, 353, 354, 355, 356, 357, 358, 359, 360, 361, 362, 364, 365, 366, 367, 368, 369, 370, 371, 372, 373, 374, 375, 376, 377, 378, 379, 380, 381, 382, 383, 384, 385, 386, 387, 388, 389, 390, 391, 392, 393, 394, 395, 396, 397, 398, 399, 400, 401, 402, 403, 404, 405, 406, 407, 408, 409, 410, 411, 412, 413, 414, 415, 416, 417, 418, 419, 420, 421, 422, 423, 424, 425, 426, 427, 428, 429, 430, 431, 432, 433, 434, 435, 436, 437, 438, 439, 440, 441, 442, 443, 444) "Jira"
QDEBUG : KSyntaxHighlighting::RepositoryTest::testIncludedFormats() syntaxrepository_test(4584)/(default) ?[31m?[34mKSyntaxHighlighting::RepositoryTest::testIncludedFormats?[0m: QVector(1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25, 26, 27, 28, 29, 30, 31, 32, 33, 34, 35, 36, 37, 38, 39, 40, 41, 42, 43, 44, 45, 46, 47, 48, 49, 50, 51, 52, 53, 54, 55, 56, 57, 58, 59, 60, 61, 62, 63, 64, 65, 66, 67, 68, 69, 70, 71, 72, 73, 74, 75, 76, 77, 78, 79, 80, 81, 82, 83, 84, 85, 86, 87, 88, 89, 90, 91, 92, 93, 94, 95, 96, 97, 98, 99, 100, 101, 102, 103, 104, 105, 106, 107, 108, 109, 110, 111, 112, 113, 114, 115, 116, 117, 118, 119, 120, 121, 122, 123, 124, 125, 126, 127, 128, 129, 130, 131, 132, 133, 134, 135, 136, 137, 138, 139, 140, 141, 142, 143, 144, 145, 146, 147, 148, 149, 150, 151, 152, 153, 154, 155, 156, 157, 158, 159, 160, 161, 162, 163, 164, 165, 166, 167, 168, 169, 170, 171, 172, 173, 174, 175, 176, 177, 178, 179, 180, 181, 182, 183, 184, 185, 186, 187, 188, 189, 190, 191, 192, 193, 194, 195, 196, 197, 198, 199, 200, 201, 202, 203, 204, 205, 206, 207, 208, 209, 210, 211, 212, 213, 214, 215, 216, 217, 218, 219, 220, 221, 222, 223, 224, 225, 226, 227, 228, 229, 230, 231, 232, 233, 234, 235, 236, 237, 238, 239, 240, 241, 242, 243, 244, 245, 246, 247, 248, 249, 250, 251, 252, 253, 254, 255, 256, 257, 258, 259, 260, 261, 262, 263, 264, 265, 266, 267, 268, 269, 270, 271, 272, 273, 274, 275, 276, 277, 278, 279, 280, 281, 282, 283, 284, 285, 286, 287, 288, 289, 290, 291, 292, 293, 294, 295, 296, 297, 298, 299, 300, 301, 302, 303, 304, 305, 306, 307, 308, 309, 310, 311, 312, 313, 314, 315, 316, 317, 318, 319, 320, 321, 322, 323, 324, 325, 326, 327, 328, 329, 330, 331, 332, 333, 334, 335, 336, 337, 338, 339, 340, 341, 342, 343, 344, 345, 346, 347, 348, 349, 350, 351, 352, 353, 354, 355, 356, 357, 358, 359, 360, 361, 362, 363, 364, 365, 366, 367, 368, 369, 370, 371, 372, 373, 374, 375, 376, 377, 378, 379, 380, 381, 382, 383, 384, 385, 386, 387, 388, 389, 390, 391, 392, 393, 394, 395, 396, 397, 398, 399, 400, 401, 402, 403, 404, 405, 406, 407, 408, 409, 410, 411, 412, 413, 414, 415, 416, 417, 418, 419, 420, 421, 422, 423, 424, 425, 426, 427, 428, 429, 430, 431, 432, 433, 434, 435, 436, 437, 438, 439, 440, 441, 442, 443)

Difference is: 363 is missing in the first version.

Btw., the test is a bit easier to read with:

// collect all formats, shall be numbered from 1..
QSet<int> formatIds;
for (auto d : qAsConst(includedDefs)) {
    const auto formats = d.formats();
    for (const auto format : formats) {
        // no duplicates
        QVERIFY(!formatIds.contains(format.id()));
        formatIds.insert(format.id());
    }
}

// ensure all ids are there from 1..size
for (int i = 1; i <= formatIds.size(); ++i) {
    printf ("id %d\n", i);
    QVERIFY(formatIds.contains(i));
}

Interesting: alert_indent.xml has no rules except one IncludeRules (And I am supposed to be the author?!):

<language version="4" kateversion="2.4" name="Alerts_indent" section="Other" extensions="" mimetype="" author="Dominik Haumann (dhaumann@kde.org)" license="MIT" hidden="true">
<highlighting>
    <contexts>
    <context attribute="Normal Text" lineEndContext="#pop" name="Normal Text" >
        <IncludeRules context="##Alerts" />
    </context>
    </contexts>
    <itemDatas>
    <itemData name="Normal Text" defStyleNum="dsNormal"/>
    </itemDatas>
</highlighting>
<general>
    <folding indentationsensitive="1" />
</general>
</language>

From kate.git:

commit 6c9ee3c58549d6cad46b9a42cf7407d25cdfa62e
Author: Joseph Wenninger <kde@jowenn.at>
Date: Thu Sep 17 23:33:39 2009 +0000

I'm temporarily adding an alert highlighting which has the indentation based folding flag set, so that python folding "works" (at least a littlebit) again.
In the end the real solution should be to allow mixing of indentation based parts with non indentation based ones.

svn path=/trunk/KDE/kdelibs/kate/; revision=1025083

Still the question is: why is that not seen in includedDefinitions. That hl ist not at all found, thought it is included.

Yes:

  1. Why don't we see Alert_indent.xml
  2. Is Alert_indent.xml still needed with the new KSyntaxHighlighting framework?

I think the issue is, that if you have just include rules and a context with include rules, you will skip that one context completely during including.
Therefore it is missing in the end result, I assume.

Given that hl definition is "eaten" during the include cycle, I guess one can just delete it and directly include the other one.

dhaumann updated this revision to Diff 40172.Aug 21 2018, 7:25 PM
  • Simplify code

The problem likely was that Alerts.xml does not have the indentationBasedFolding attribute set. But it's needed in e.g. Python.

dh@eriador:syntax :-) (check-full-formats) $ grep Alerts_indent *
alert_indent.xml:<language version="4" kateversion="2.4" name="Alerts_indent" section="Other" extensions="" mimetype="" author="Dominik Haumann (dhaumann@kde.org)" license="MIT" hidden="true">
coffee.xml:        <IncludeRules context="##Alerts_indent"/>
coffee.xml:        <IncludeRules context="##Alerts_indent"/>
pig.xml:                                <IncludeRules context="##Alerts_indent" />
prolog.xml:             <IncludeRules context="##Alerts_indent" />
prolog.xml:             <IncludeRules context="##Alerts_indent" />
python.xml:                             <IncludeRules context="##Alerts_indent" />
python.xml:                             <IncludeRules context="##Alerts_indent" />
python.xml:                             <IncludeRules context="##Alerts_indent" />
python.xml:                             <IncludeRules context="##Alerts_indent" />

I would be in favor of removing Alerts_indent.
It has no use ATM. Even if the indentation setting would be important, we can't read that value, as no format of that definition is ever ending up in any textline.

True, agreed. And it was an ugly workaround in the first place.

dhaumann updated this revision to Diff 40349.Aug 24 2018, 6:00 AM
  • Verify Definition::formats() contain all formats from 1...N
cullmann accepted this revision.Aug 24 2018, 6:25 AM
This revision is now accepted and ready to land.Aug 24 2018, 6:25 AM
This revision was automatically updated to reflect the committed changes.