Add CopyPaste unit test for spreadsheet
ClosedPublic

Authored by shubham on Dec 30 2019, 4:13 PM.

Diff Detail

Repository
R262 LabPlot
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
shubham created this revision.Dec 30 2019, 4:13 PM
Restricted Application added a project: KDE Edu. · View Herald TranscriptDec 30 2019, 4:13 PM
Restricted Application added a subscriber: kde-edu. · View Herald Transcript
shubham requested review of this revision.Dec 30 2019, 4:13 PM

@asemke Any help why it is failing to compile? I have already added the directory to cmake.

@asemke Any help why it is failing to compile? I have already added the directory to cmake.

You also need to add the appropriate CMakeLists.txt in tests/spreadsheet/. Just copy this file from another sub-folder in tests and adjust it.

apol added a subscriber: apol.Dec 31 2019, 6:43 PM
apol added inline comments.
compile
11 ↗(On Diff #72389)

Changes in compile and compile_debug feel wrong and in any case unrelated to the rest.

shubham updated this revision to Diff 72535.Jan 1 2020, 2:36 PM
asemke added inline comments.Jan 2 2020, 4:24 PM
tests/spreadsheet/CopyPasteTest.cpp
91 ↗(On Diff #72535)

you named your class CopyPasteTest since we planned to test the copy&paste functionality in the spreadsheet. This testCopyPaste() test is the only test for this. All other tests are kind of trivial and can be either removed or moved to another file where we'd test more general features in the speadsheet. Let's keep this file for copy&paste only and add more tests to cover the different behaviors, and there are many of them...

102 ↗(On Diff #72535)

this comment can be removed.

106 ↗(On Diff #72535)

why is this commented out?

shubham updated this revision to Diff 73485.Jan 14 2020, 6:47 AM
shubham retitled this revision from Add unit test for spreadsheet to Add CopyPaste unit test for spreadsheet.
shubham added inline comments.
tests/spreadsheet/CopyPasteTest.cpp
106 ↗(On Diff #72535)

Because it is saying error: ‘void SpreadsheetView::pasteIntoSelection()’ is private within this context. Any help?

asemke added inline comments.Jan 14 2020, 7:17 AM
tests/spreadsheet/CopyPasteTest.cpp
106 ↗(On Diff #72535)

ok. Then just make this function a public slot in SpreadsheetView.h.

shubham updated this revision to Diff 73539.Jan 14 2020, 3:25 PM

Compiles, but make test hangs on testing CopyPasteTest

shubham updated this revision to Diff 73551.Jan 14 2020, 5:22 PM

Bump copyright year

Compiles, but make test hangs on testing CopyPasteTest

Can you add some debug output to this test to see where it gets stuck?

@asemke Actually, I waited for the test to finish and it failed with the following output.

      Start 20: copypastetest
20/20 Test #20: copypastetest ....................***Failed   61.28 sec
SignallingUndoCommand: failed to copy unknown type const AbstractColumn* (needs to be registered with qRegisterMetaType())!

QMetaObject::invokeMethod: No such method Column::rowsAboutToBeInserted()
Candidates are:
    rowsAboutToBeInserted(const AbstractColumn*,int,int)
FAILED to invoke rowsAboutToBeInserted on Column

SignallingUndoCommand: failed to copy unknown type const AbstractColumn* (needs to be registered with qRegisterMetaType())!

QMetaObject::invokeMethod: No such method Column::rowsInserted()
Candidates are:
    rowsInserted(const AbstractColumn*,int,int)
FAILED to invoke rowsInserted on Column

SignallingUndoCommand: failed to copy unknown type const AbstractColumn* (needs to be registered with qRegisterMetaType())!

QMetaObject::invokeMethod: No such method Column::rowsAboutToBeInserted()
Candidates are:
    rowsAboutToBeInserted(const AbstractColumn*,int,int)
FAILED to invoke rowsAboutToBeInserted on Column

SignallingUndoCommand: failed to copy unknown type const AbstractColumn* (needs to be registered with qRegisterMetaType())!

QMetaObject::invokeMethod: No such method Column::rowsInserted()
Candidates are:
    rowsInserted(const AbstractColumn*,int,int)
FAILED to invoke rowsInserted on Column

@asemke Actually, I waited for the test to finish and it failed with the following output.

      Start 20: copypastetest
20/20 Test #20: copypastetest ....................***Failed   61.28 sec
SignallingUndoCommand: failed to copy unknown type const AbstractColumn* (needs to be registered with qRegisterMetaType())!

QMetaObject::invokeMethod: No such method Column::rowsAboutToBeInserted()
Candidates are:
    rowsAboutToBeInserted(const AbstractColumn*,int,int)
FAILED to invoke rowsAboutToBeInserted on Column

SignallingUndoCommand: failed to copy unknown type const AbstractColumn* (needs to be registered with qRegisterMetaType())!

QMetaObject::invokeMethod: No such method Column::rowsInserted()
Candidates are:
    rowsInserted(const AbstractColumn*,int,int)
FAILED to invoke rowsInserted on Column

SignallingUndoCommand: failed to copy unknown type const AbstractColumn* (needs to be registered with qRegisterMetaType())!

QMetaObject::invokeMethod: No such method Column::rowsAboutToBeInserted()
Candidates are:
    rowsAboutToBeInserted(const AbstractColumn*,int,int)
FAILED to invoke rowsAboutToBeInserted on Column

SignallingUndoCommand: failed to copy unknown type const AbstractColumn* (needs to be registered with qRegisterMetaType())!

QMetaObject::invokeMethod: No such method Column::rowsInserted()
Candidates are:
    rowsInserted(const AbstractColumn*,int,int)
FAILED to invoke rowsInserted on Column

Ok. I missed this part. Please look at the qRegisterMetaType calls in other tests, for example in AsciiFilterTest::initTestCase(). We need to add this here, too.

shubham updated this revision to Diff 73854.Jan 19 2020, 7:43 AM

Add qRegisterMetaType() calls

shubham marked an inline comment as done.Jan 19 2020, 7:44 AM

@asemke But the test still fails with:

********* Start testing of CopyPasteTest *********
Config: Using QtTest library 5.13.2, Qt 5.13.2 (x86_64-little_endian-lp64 shared (dynamic) release build; by GCC 7.4.0)
PASS   : CopyPasteTest::initTestCase()
KCrash: crashing... crashRecursionCounter = 2
KCrash: Application Name = copypastetest path = /home/aryan/labplot/build/tests/spreadsheet pid = 5055
KCrash: Arguments: /home/aryan/labplot/build/tests/spreadsheet/copypastetest 
KCrash: Attempting to start /usr/lib/x86_64-linux-gnu/libexec/drkonqi from kdeinit
sock_file=/run/user/1000/kdeinit5__0
QWARN  : CopyPasteTest::testCopyPaste() QSocketNotifier: Invalid socket 9 and type 'Read', disabling...

@asemke But the test still fails with:

********* Start testing of CopyPasteTest *********
Config: Using QtTest library 5.13.2, Qt 5.13.2 (x86_64-little_endian-lp64 shared (dynamic) release build; by GCC 7.4.0)
PASS   : CopyPasteTest::initTestCase()
KCrash: crashing... crashRecursionCounter = 2
KCrash: Application Name = copypastetest path = /home/aryan/labplot/build/tests/spreadsheet pid = 5055
KCrash: Arguments: /home/aryan/labplot/build/tests/spreadsheet/copypastetest 
KCrash: Attempting to start /usr/lib/x86_64-linux-gnu/libexec/drkonqi from kdeinit
sock_file=/run/user/1000/kdeinit5__0
QWARN  : CopyPasteTest::testCopyPaste() QSocketNotifier: Invalid socket 9 and type 'Read', disabling...

It crashes in line 50 at

sheet->column(2)->setValueAt(0, 1000.0);

On default, the spreadsheet is created with two columns. Here you want to set the value in the third column that is not available. If you want to work with more than two columns, you need to add more columns first. You can use spreadsheet->setColumnCount() to set the number of columns.

shubham updated this revision to Diff 73871.Jan 19 2020, 12:55 PM

Few changes, test still fails. I tried debugging it and check the stack trace, it seems to end in pasteIntoSelection()

Few changes, test still fails. I tried debugging it and check the stack trace, it seems to end in pasteIntoSelection()

Change the string to

const QString str = "10.0 100.0\n20.0 200.0\n30.0 300.0";

So, just remove the white spaces in it. Nevertheless, we shouldn't crash in this case. I'll check this.

In your test you populate the spreadsheet with some values and then you paste the same values again. I'm not sure it was intended.

@asemke I got a bit confuse, can you please assist a bit?

@asemke I got a bit confuse, can you please assist a bit?

You found a bug in our code with your test. I'll fix it soon. Until then please use a "well behaving" data to be copied. So, instead of

const QString str = "10.0 100.0 \n 20.0 200.0  \n 30.0 300.0";

simpy use

const QString str = "10.0 100.0\n20.0 200.0\n30.0 300.0";

The most simple test cases would look like

/*!
   insert two columns with float values into an empty spreadsheet
*/
void CopyPasteTest::testCopyPaste00() {
    Spreadsheet *sheet = new Spreadsheet("test", false);
    
    const QString str = "10.0 100.0\n20.0 200.0";
    
    QApplication::clipboard()->setText(str);

    SpreadsheetView *view = new SpreadsheetView(sheet, false);
    view->pasteIntoSelection();

    //check the column modes
    QCOMPARE(spreadsheet.column(0)->columnMode(), AbstractColumn::Numeric);
    QCOMPARE(spreadsheet.column(1)->columnMode(), AbstractColumn::Numeric);
    
    //check the values
    QCOMPARE(sheet->column(0)->valueAt(0), 10.0);
    QCOMPARE(sheet->column(1)->valueAt(0), 100.0);
    QCOMPARE(sheet->column(0)->valueAt(1), 20.0);
    QCOMPARE(sheet->column(1)->valueAt(2), 200.0);
}


/*!
   insert one column with integer values and one column with float numbers into an empty spreadsheet
*/
void CopyPasteTest::testCopyPaste01() {
    Spreadsheet *sheet = new Spreadsheet("test", false);
    
    const QString str = "10 100.0\n20 200.0";
    
    QApplication::clipboard()->setText(str);

    SpreadsheetView *view = new SpreadsheetView(sheet, false);
    view->pasteIntoSelection();

    //check the column modes
    QCOMPARE(spreadsheet.column(0)->columnMode(), AbstractColumn::Integer);
    QCOMPARE(spreadsheet.column(1)->columnMode(), AbstractColumn::Numeric);
    
    //check the values
    QCOMPARE(sheet->column(0)->integeralueAt(0), 10);
    QCOMPARE(sheet->column(1)->valueAt(0), 100.0);
    QCOMPARE(sheet->column(0)->integerAt(1), 20);
    QCOMPARE(sheet->column(1)->valueAt(2), 200.0);
}

etc.

asemke added a comment.Sun, Feb 2, 5:22 PM

@asemke I got a bit confuse, can you please assist a bit?

do you need more help on this?

This revision was not accepted when it landed; it landed in state Needs Review.Sun, Feb 23, 10:45 AM
This revision was automatically updated to reflect the committed changes.