- Test also the creation and deletion of 100 files.
- Use QTRY_COMPARE and QTRY_VERIFY to not increase wait times.
Details
Details
- Reviewers
dfaure - Group Reviewers
Frameworks - Commits
- R241:0c8cd4076b7d: kdirlistertest doesn't fail at random
Repeating the test several times, it failed sometimes due to timing problems, doesn't fail any more for me.
Diff Detail
Diff Detail
- Repository
- R241 KIO
- Lint
Automatic diff as part of commit; lint not applicable. - Unit
Automatic diff as part of commit; unit tests not applicable.
| autotests/kdirlistertest.cpp | ||
|---|---|---|
| 194 | Should be QStringLiteral("") instead of QString(""). | |
Comment Actions
I'm all for making tests more stable, but changing what the test is testing, sounds dangerous to me (it might hide bugs in the cases that the test intended to be testing). Sometimes it's necessary to change what we're testing, if we're testing something unreliable, but e.g. creating one file should and must reliably trigger an update, we need to make sure of that.
| autotests/kdirlistertest.cpp | ||
|---|---|---|
| 183 | That's testing a different use case. Isn't there a risk that this hides a bug when a single file is created by the user (which is more common than 1000...)? | |
| 293 | Why 4s? The comment sounds like it has to be 4 exactly, but I guess this is just "safety"? That's a lot, it makes the test quite slow. (Rule of thumb is that a unittest should last less than 5s) | |
| 502 | This URL is incorrect, it's missing a slash, you're referring to a hostname of "index.html" here. But if you fix it, how then could this ever be anything else than text/html? I don't see the point of this indirection. | |
Comment Actions
Add the new tests instead of replace.
Use QTRY_COMPARE instead of custom loops.
Reduce the number of files to 100.
In my machine, the mime type of file.html is "application/x-extension-html"
because of
Comment Actions
Please add QStandardPaths::setTestModeEnabled(true) in initTestCase() so that your (broken, thank you WinE) locally defined mimetypes don't interfer with the test. Then "hardcoding" text/html will be fine again.
| autotests/kdirlistertest.cpp | ||
|---|---|---|
| 222 | 100 new files, you mean? ;) | |
| 236 | (so 0 would be fine?) | |
| 516 | with QTRY_* and/or QSignalSpy::wait(), the enterLoop/exitLoop old solution isn't really necessary anymore | |
| 534 | convert to new style connect, as you did for the other code you touched? | |
| 608 | QTRY_VERIFY(!m_refreshedItems.isEmpty()); (same for all other instances) | |
Comment Actions
Removed the use of enterLoop and exitLoop in favor of QTRY_COMPARE and QTRY_VERIFY.
Removed some waits
Changed all the connections to the new syntax.
Have run it several times and it hasn't failed here.
| autotests/kdirlistertest.cpp | ||
|---|---|---|
| 375 | that's immediately true since it's 0 initially, so the TRY_ brings nothing. It's not like it's going to down if we wait ;) Same for the lines before and after, actually. | |
| 453–454 | I keep thinking that this would be more readable as QTRY_VERIFY(!m_refreshedItems.isEmpty()) But YMMV. | |
| 480–481 | How? This test is deleting a single file... I don't get it. | |
| 513 | The problem is, this will likely move on after the first signal is emitted. I think we need to sum up the number of items emitted by the deleted signals and wait until we reach 100. | |
| 924 | there's nothing to wait for here, no TRY_ needed. | |
| 943 | This is one case where the new code isn't equivalent to the old code. Starting with an empty list, "wait until it's empty" will move on immediately, while the old code was doing "wait a second and ensure the list is still empty afterwards". | |
| 980 | We just waited for the completed signal, why wait again? I don't think this TRY_ is needed. | |
| 981 | no TRY_ needed | |
| 990 | same here, this isn't async, it's done by the redirection handling, which we just waited for. | |
| 1015 | as above | |
| 1065 | any QTRY_COMPARE(spy.count(), 0) is pretty useless to me. It's 0 already, we'll never wait. (or if it's not 0, we'll wait and fail, instead of failing right away) | |
Comment Actions
I'm never sure at how many lines the comments affects, but the last one should be:
QTRY_COMPARE(m_dirLister.spyClear.count(), 1); QCOMPARE(m_dirLister.spyClearQUrl.count(), 0);
Otherwise the test fails for me.
Comment Actions
Those comments were all for a single line of code ;)
But yes, your last change is exactly what I meant. "wait for nothing" is when waiting for a spy to get to 0.
The line above that waits for it to get to 1, so TRY_ makes sense there.
Thanks!