D20264: Add test for "Auto Reload Document" option

[Dominik Haumann]

dhaumann added a comment.


  Yes, that's exactly what I meant. But if it's not stable, then it does not 
help. Maybe in that case keep it as is for now?

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D20264

To: loh.tar, dhaumann, cullmann
Cc: cullmann, kwrite-devel, kde-frameworks-devel, #ktexteditor, domson, 
michaelh, ngraham, bruns, demsking, sars, dhaumann

D20264: Add test for "Auto Reload Document" option

[loh tar]

loh.tar added a comment.


  @dhaumann asked elsewhere
  
  > Instead of waiting so long, we could alternatively also have a while loop 
waiting up to 20 times for 50ms. This way, the test would be faster if possible.
  
  Like this?
  
diff --git a/autotests/src/katedocument_test.cpp 
b/autotests/src/katedocument_test.cpp
index 4be9d7c3..e24a8ad9 100644
--- a/autotests/src/katedocument_test.cpp
+++ b/autotests/src/katedocument_test.cpp
@@ -655,14 +655,23 @@ void KateDocumentTest::testAutoReload()
// without to wait before it doesn't work here. It mostly fails with 
500,
// it tend to work with 600, but is not guarantied to work even with 800
QTest::qWait(1000);
+// No idea how to do some slice test here

stream << "\nTest";
stream.flush();

- // Hardcoded delay m_modOnHdTimer in DocumentPrivate
+// Try not to waste more time than needed, but we need to wait
+const int timeSlice = 50;
+// Hardcoded delay m_modOnHdTimer in DocumentPrivate
// + min val with which it looks working reliable here
-QTest::qWait(200 + 800);
-QCOMPARE(doc.text(), "Hello\nTest");
+const int totalPatience = 200 + 800;
+
+QString refTxT = ("Hello\nTest");
+for (int w = 0; w <= totalPatience; w += timeSlice) {
+QTest::qWait(timeSlice);
+if (doc.text() == refTxT) break;
+}
+QCOMPARE(doc.text(), refTxT);
// ...stay in the last line after reload!
QCOMPARE(view->cursorPosition().line(), doc.documentEnd().line());

@@ -674,8 +683,12 @@ void KateDocumentTest::testAutoReload()
stream.flush();
file.close();

-QTest::qWait(200 + 800);
-QCOMPARE(doc.text(), "Hello\nTest\nWorld!");
+refTxT = ("Hello\nTest\nWorld!");
+for (int w = 0; w <= totalPatience; w += timeSlice) {
+QTest::qWait(timeSlice);
+if (doc.text() == refTxT) break;
+}
+QCOMPARE(doc.text(), refTxT);
// ...and ensure we have not move around
QCOMPARE(view->cursorPosition(), c);
}
  
  No Patch
  
Totals: 3 passed, 0 failed, 0 skipped, 0 blacklisted, 3152ms
  
  With Patch
  
Totals: 3 passed, 0 failed, 0 skipped, 0 blacklisted, 2619ms
533ms saved
  
  But it can still fail now :-(
  
Totals: 2 passed, 1 failed, 0 skipped, 0 blacklisted, 3127ms
  
  also with increased waiting times
  
Totals: 2 passed, 1 failed, 0 skipped, 0 blacklisted, 3787ms
Totals: 2 passed, 1 failed, 0 skipped, 0 blacklisted, 4802ms
  
  Does maybe the check "if (doc.text() == " disturbs the nice waiting?
  Playing with longer slice shows an effect around a 500ms slice, but then we 
have no benefit
  
Totals: 3 passed, 0 failed, 0 skipped, 0 blacklisted, 3147ms

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D20264

To: loh.tar, dhaumann, cullmann
Cc: cullmann, kwrite-devel, kde-frameworks-devel, #ktexteditor, domson, 
michaelh, ngraham, bruns, demsking, sars, dhaumann

D20264: Add test for "Auto Reload Document" option

[loh tar]

This revision was automatically updated to reflect the committed changes.
Closed by commit R39:f5f715ea1441: Add test for Auto Reload 
Document option (authored by loh.tar).

CHANGED PRIOR TO COMMIT
  https://phabricator.kde.org/D20264?vs=55576=56623#toc

REPOSITORY
  R39 KTextEditor

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D20264?vs=55576=56623

REVISION DETAIL
  https://phabricator.kde.org/D20264

AFFECTED FILES
  autotests/src/katedocument_test.cpp
  autotests/src/katedocument_test.h
  src/document/katedocument.cpp

To: loh.tar, dhaumann, cullmann
Cc: cullmann, kwrite-devel, kde-frameworks-devel, #ktexteditor, domson, 
michaelh, ngraham, bruns, demsking, sars, dhaumann

D20264: Add test for "Auto Reload Document" option

[loh tar]

loh.tar added a comment.


  I spend a couple of time for this stuff. 
  Would be nice someone else could try it or do some investigation with tools 
I'm not familiar with, and don't want to be atm. Without feedback I will push 
it in the next few days.

REVISION DETAIL
  https://phabricator.kde.org/D20264

To: loh.tar, dhaumann, cullmann
Cc: cullmann, kwrite-devel, kde-frameworks-devel, #ktexteditor, gennad, domson, 
michaelh, ngraham, bruns, demsking, sars, dhaumann

D20264: Add test for "Auto Reload Document" option

[Christoph Cullmann]

cullmann accepted this revision.
cullmann added a comment.


  I think this can go in as is, as long as the test works determinstically, 
which I assume it does.

REVISION DETAIL
  https://phabricator.kde.org/D20264

To: loh.tar, dhaumann, cullmann
Cc: cullmann, kwrite-devel, kde-frameworks-devel, #ktexteditor, gennad, domson, 
michaelh, ngraham, bruns, demsking, sars, dhaumann

D20264: Add test for "Auto Reload Document" option

[loh tar]

loh.tar added a comment.


  In D20264#448583 , @dhaumann wrote:
  
  > I think KateViewInternal::updateView is called for cursor blinking for 
instance. May that be an issue?
  
  
  Regarding ...
  
  > KateViewInternal::updateView is very often called
  
  ...I don't think so. It was much more frequently than some blinking. That 
observation was only a general notice, not to this test...IIRC
  So, what do with this? Commit as it is?

REVISION DETAIL
  https://phabricator.kde.org/D20264

To: loh.tar, dhaumann
Cc: kwrite-devel, kde-frameworks-devel, #ktexteditor, gennad, domson, michaelh, 
ngraham, bruns, demsking, cullmann, sars, dhaumann

D20264: Add test for "Auto Reload Document" option

[Dominik Haumann]

dhaumann added a comment.


  I think KateViewInternal::updateView is called for cursor blinking for 
instance. May that be an issue?

REVISION DETAIL
  https://phabricator.kde.org/D20264

To: loh.tar, dhaumann
Cc: kwrite-devel, kde-frameworks-devel, #ktexteditor, gennad, domson, michaelh, 
ngraham, bruns, demsking, cullmann, sars, dhaumann

D20264: Add test for "Auto Reload Document" option

[loh tar]

loh.tar updated this revision to Diff 55576.
loh.tar edited the summary of this revision.
loh.tar added a comment.


  - Enhance the test by checks for proper cursor position
  
  While adding your tiny change, it pointed out, that the test was not correct 
regarding needed waiting times. Pls compare new/old comments about the delays 
:-/ Any idea to the magic value?
  
  I noticed..
  
  - that slotDelayedHandleModOnHd is still called/processed. I'm not sure if 
that's OK
  - KateViewInternal::updateView is very often called and so 
doc()->delayAutoReload() too. The intend was to block the reload while 
scrolling around. Perhaps could that blocking somewhere else better placed

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D20264?vs=55468=55576

REVISION DETAIL
  https://phabricator.kde.org/D20264

AFFECTED FILES
  autotests/src/katedocument_test.cpp
  autotests/src/katedocument_test.h
  src/document/katedocument.cpp

To: loh.tar, dhaumann
Cc: kwrite-devel, kde-frameworks-devel, #ktexteditor, gennad, domson, michaelh, 
ngraham, bruns, demsking, cullmann, sars, dhaumann

D20264: Add test for "Auto Reload Document" option

[Dominik Haumann]

dhaumann accepted this revision.
dhaumann added a comment.
This revision is now accepted and ready to land.


  There is a getter bool KateGlobal::self()->unitTestMode(). You could use this 
in DocumentPrivate to reduce the timer from 3000 to e.g. 50ms or so to reduce 
testing time.
  
  But besides that, this looks good to me, thanks for working on this!

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D20264

To: loh.tar, dhaumann
Cc: kwrite-devel, kde-frameworks-devel, #ktexteditor, gennad, domson, michaelh, 
ngraham, bruns, demsking, cullmann, sars, dhaumann

D20264: Add test for "Auto Reload Document" option

[loh tar]

loh.tar created this revision.
loh.tar added a reviewer: dhaumann.
Herald added projects: Kate, Frameworks.
Herald added subscribers: kde-frameworks-devel, kwrite-devel.
loh.tar requested review of this revision.

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D20264

AFFECTED FILES
  autotests/src/katedocument_test.cpp
  autotests/src/katedocument_test.h

To: loh.tar, dhaumann
Cc: kwrite-devel, kde-frameworks-devel, #ktexteditor, gennad, domson, michaelh, 
ngraham, bruns, demsking, cullmann, sars, dhaumann