Closed brong closed 7 years ago
brong
commented
12 years ago From: Bron Gondwana
We should have a branch which either passes on master, or only fails on currently known bugs that will be fixed before releasing 2.5.
I'd like to also set up automated builds on a bunch of different platforms, but that's a separate task.
brong
commented
12 years ago From: Greg Banks
I did some work on this late last year, and got Cassandane to the point where only a handful of tests failed against Cyrus' cmu/master. Partly I did this by disabling tests and suites using the filtering mechanism. I'll flush any pending commits I can find and post the output of run vs cmu/master to demonstrate.
In terms of policy I think the best thing to do is to mirror the Cyrus branch structure in Cassandane. In particular, we should expect the Cassandane master branch to always be able to pass against the Cyrus master branch, and to have a Jenkins instance enforcing that.
brong
commented
12 years ago From: Greg Banks
Running today's Cassandane cmu/master vs today's Cyrus cmu/master I get the following nine problems.
There were 5 errors: 1) Cassandane/Instance.pm:723 - test_add_annot_deliver(Cassandane::Cyrus::Annotator) child process (binary /home/gnb/software/cyrus/cassandane/utils/annotator.pl pid 24979) exited with code 255 2) Cassandane/Instance.pm:723 - test_set_system_flag_deliver(Cassandane::Cyrus::Annotator) child process (binary /home/gnb/software/cyrus/cassandane/utils/annotator.pl pid 24999) exited with code 255 3) Cassandane/Instance.pm:723 - test_add_annot_deliver_tomailbox(Cassandane::Cyrus::Annotator) child process (binary /home/gnb/software/cyrus/cassandane/utils/annotator.pl pid 25019) exited with code 255 4) Cassandane/Instance.pm:723 - test_set_user_flag_deliver(Cassandane::Cyrus::Annotator) child process (binary /home/gnb/software/cyrus/cassandane/utils/annotator.pl pid 25039) exited with code 255 5) Cassandane/Cyrus/Rename.pm:155 - test_rename_user(Cassandane::Cyrus::Rename) IMAP Command : 'rename' failed. Response was : no - Unknown code ____ 255 ...propagated
There were 4 failures: 1) Cassandane/Cyrus/Bug3470.pm:109 - test_list_percent(Cassandane::Cyrus::Bug3470) LIST data mismatch: $VAR1 = [ [ [ '\Noinferiors', '\HasNoChildren' ], '/', 'INBOX' ], [ [ '\Noselect', '\HasChildren' ], '/', '2001' ], [ [ '\HasNoChildren' ], '/', 'Drafts' ] ];
2) Cassandane/Cyrus/Quota.pm:120 - test_upgrade_v2_4(Cassandane::Cyrus::Quota) Structures begin differing at: $a->[1] = '10' $b->[1] = '0'
3) Cassandane/Cyrus/Quota.pm:120 - test_using_annotstorage_msg_late(Cassandane::Cyrus::Quota) Structures begin differing at: $a->[1] = '51' $b->[1] = '103'
4) Cassandane/Cyrus/Quota.pm:120 - test_using_annotstorage_msg(Cassandane::Cyrus::Quota) Structures begin differing at: $a->[1] = '50' $b->[1] = '102'
Test was not successful.
brong
commented
12 years ago From: Greg Banks
> There were 5 errors: > 1) Cassandane/Instance.pm:723 - test_add_annot_deliver(Cassandane::Cyrus::Annotator) > child process (binary /home/gnb/software/cyrus/cassandane/utils/annotator.pl pid 24979) exited with code 255 > 2) Cassandane/Instance.pm:723 - test_set_system_flag_deliver(Cassandane::Cyrus::Annotator) > child process (binary /home/gnb/software/cyrus/cassandane/utils/annotator.pl pid 24999) exited with code 255 > 3) Cassandane/Instance.pm:723 - test_add_annot_deliver_tomailbox(Cassandane::Cyrus::Annotator) > child process (binary /home/gnb/software/cyrus/cassandane/utils/annotator.pl pid 25019) exited with code 255 > 4) Cassandane/Instance.pm:723 - test_set_user_flag_deliver(Cassandane::Cyrus::Annotator) > child process (binary /home/gnb/software/cyrus/cassandane/utils/annotator.pl pid 25039) exited with code 255
These are all due to utils/annotator.pl using the old API. Fixed in commit "Update annotator.pl to latest API"
> 5) Cassandane/Cyrus/Rename.pm:155 - test_rename_user(Cassandane::Cyrus::Rename) > IMAP Command : 'rename' failed. Response was : no - Unknown code ____ 255 ...propagated
This is a regression in Cyrus, introduced in commit "lib: create cyrus_copyfile API"
> There were 4 failures: > 1) Cassandane/Cyrus/Bug3470.pm:109 - test_list_percent(Cassandane::Cyrus::Bug3470) > LIST data mismatch: $VAR1 = [ > [...]
Don't know yet.
> 2) Cassandane/Cyrus/Quota.pm:120 - test_upgrade_v2_4(Cassandane::Cyrus::Quota) > Structures begin differing at: > $a->[1] = '10' > $b->[1] = '0' >
Don't know yet.
> > 3) Cassandane/Cyrus/Quota.pm:120 - test_using_annotstorage_msg_late(Cassandane::Cyrus::Quota) > Structures begin differing at: > $a->[1] = '51' > $b->[1] = '103' > > 4) Cassandane/Cyrus/Quota.pm:120 - test_using_annotstorage_msg(Cassandane::Cyrus::Quota) > Structures begin differing at: > $a->[1] = '50' > $b->[1] = '102' >
This is due to the test not handling delayed expunge mode. Fixed in commit "Handle delayed expunge mode in Quota tests."
brong
commented
12 years ago From: Bron Gondwana
> 5) Cassandane/Cyrus/Rename.pm:155 - test_rename_user(Cassandane::Cyrus::Rename) > IMAP Command : 'rename' failed. Response was : no - Unknown code ____ 255 ...propagated
This is now fixed in the commit "mailbox: add mkdir back to copyfile, and return IOERROR"
> There were 4 failures: > 1) Cassandane/Cyrus/Bug3470.pm:109 - test_list_percent(Cassandane::Cyrus::Bug3470) > LIST data mismatch: $VAR1 = [ > [...]
I've fixed this one in commit "Bug3470 - fix LSUB test".
The difference is that it now ejects a "\HasNoChildren" as well as a "\Noinferiors". Strictly speaking either is probably fine - you can infer the \HasNoChildren anyway.
RFC 3348 doesn't have an opinion on the topic, only that \HasChildren can't be combined with either of the above (obviously).
> 2) Cassandane/Cyrus/Quota.pm:120 - test_upgrade_v2_4(Cassandane::Cyrus::Quota) > Structures begin differing at: > $a->[1] = '10' > $b->[1] = '0'
Neither do I - but it's a shithouse error message. The location it's reported at is useless, and the content of the message is even more useless :(
brong
commented
12 years ago From: Julien Coloos
> > 2) Cassandane/Cyrus/Quota.pm:120 - test_upgrade_v2_4(Cassandane::Cyrus::Quota) > > Structures begin differing at: > > $a->[1] = '10' > > $b->[1] = '0' > > Neither do I - but it's a shithouse error message. The location it's reported > at is useless, and the content of the message is even more useless :(
I think it's related to the part that would have to compute the current annotation storage usage when upgrading from previous (2.4) mailbox format to new format. See imap/upgrade_index.c:upgrade_index
But at some point there were discussions about users having to call explicitely the quota utility to resync the actual usage after upgrading. So maybe the test can be dropped now ?
As for the useless message, there may be something to rethink here as after using more and more Test::Unit I noticed that negative side of it: when doing the same kind of test hundreds of times, you start to refactor and share code (easier to maintain it later); but since you only get the line of code that failed (which is what happened here), you want to add some kind of context in the message; but then if you display your message, you do not display automatically the values that were tested. Actually in such cases I think both the values being tested and the specific message explaining what was being checked are useful.
brong
commented
12 years ago From: Greg Banks
(In reply to comment #4) > > 5) Cassandane/Cyrus/Rename.pm:155 - test_rename_user(Cassandane::Cyrus::Rename) > > IMAP Command : 'rename' failed. Response was : no - Unknown code ____ 255 ...propagated > > This is now fixed in the commit "mailbox: add mkdir back to copyfile, and > return IOERROR"
Thanks.
> > 1) Cassandane/Cyrus/Bug3470.pm:109 - test_list_percent(Cassandane::Cyrus::Bug3470) > > LIST data mismatch: $VAR1 = [ > > [...] > > I've fixed this one in commit "Bug3470 - fix LSUB test".
Thanks.
> > 2) Cassandane/Cyrus/Quota.pm:120 - test_upgrade_v2_4(Cassandane::Cyrus::Quota) > > Structures begin differing at: > > $a->[1] = '10' > > $b->[1] = '0' > > Neither do I - but it's a shithouse error message. The location it's reported > at is useless, and the content of the message is even more useless :(
Agreed - I'll go do something about this.
brong
commented
12 years ago From: Greg Banks
(In reply to comment #5)
> But at some point there were discussions about users having to call explicitely > the quota utility to resync the actual usage after upgrading. So maybe the test > can be dropped now ?
Hardly. It doesn't matter whether Cyrus code gets run automatically, or periodically from master, or periodically from cron, or manually, or any other way; if it's a necessary part of completing the scenario and getting back to a working state then we have to test it.
If 'quota' needs to be run, then the test should do so and check the reported quotas before and after.
> As for the useless message, there may be something to rethink here as after > using more and more Test::Unit I noticed that negative side of it: when doing > the same kind of test hundreds of times, you start to refactor and share code > (easier to maintain it later);
Yep.
> but since you only get the line of code that > failed (which is what happened here), you want to add some kind of context in > the message; but then if you display your message, you do not display > automatically the values that were tested.
There's a couple of things here.
Firstly, jUnit, the Java implementation of the xUnit meme, reports a complete stack trace in each failure record. This handles the problem of exceptions being thrown, not just in common test code, but deep in the guts of the Code Under Test. It may be possible to tweak Test::Unit to do that. I've already had to hack around it to make the filtering system work in a sane manner, so I'll look at it.
Secondly, we do have context, it's just not in the failure record. Each 'xlog' statement in the test emits a line to syslog which looks like:
277 $self->check_messages(\%msgs); 278 xlog "check that the usage is just below the limit"; 279 $self->_check_usages([int($expected/1024)]);
Jan 3 17:35:29 ... cassandane: =====> Cyrus::Quota[278] check that the usage is just below the limit
This is why I sprinkle 'xlog' around instead of comments. If test $name fails, I look in syslog for the line "test test_$name ending", the look backwards from that for the last xlog output, then work forward again.
Ok, it sucks but it's better than nothing.
Thirdly, Test::Unit already supports two ways of adding more context, which we hardly ever use:
$self->annotation('This text tells you useful stuff');
$self->assert($boolean, 'This text tells you useful stuff'); $self->assert_equals(1, 0, 'This text tells you useful stuff');
See Test:Unit::Assert(3pm) and Test::Unit::TestCase.
brong
commented
12 years ago From: Greg Banks
I've pushed some commits which do two things.
First, the helper functions in Quota.pm use internal data structures designed so that assert_deep_equals() failures are understandable by a human. Instead of
Structures begin differing at: $a->[1] = '10' $b->[1] = '0'
we now get
Structures begin differing at: $a->{X-ANNOTATION-STORAGE}{used} = '10' $b->{X-ANNOTATION-STORAGE}{used} = '0'
Secondly the failure message prints the stack trace (or more specifically, that portion of the stack trace between the failure and the topmost test function) like this:
There was 1 failure: 1) test_upgrade_v2_4(Cassandane::Cyrus::Quota) Structures begin differing at: $a->{X-ANNOTATION-STORAGE}{used} = '10' $b->{X-ANNOTATION-STORAGE}{used} = '0' at Cassandane/Cyrus/Quota.pm line 134 Cassandane::Cyrus::Quota::_check_usages('Cassandane::Cyrus::Quota=HASH(0x3204660)', 'storage', 8, 'message', 4, 'x-annotation-storage', 10) called at Cassandane/Cyrus/Quota.pm line 1030 Cassandane::Cyrus::Quota::test_upgrade_v2_4('Cassandane::Cyrus::Quota=HASH(0x3204660)') called at /usr/share/perl5/Test/Unit/TestCase.pm line 75 [...framework calls elided...]
brong
commented
12 years ago From: Greg Banks
That last checkin broke some of the Sieve tests, which I've fixed up again. Now we have exactly one failure against cmu/master.
...............................................................................F....................................................... Time: 1091 wallclock secs (33.71 usr 2.04 sys + 161.75 cusr 5.55 csys = 203.05 CPU)
!!!FAILURES!!! Test Results: Run: 134, Failures: 1, Errors: 0
There was 1 failure: 1) test_upgrade_v2_4(Cassandane::Cyrus::Quota) Structures begin differing at: $a->{X-ANNOTATION-STORAGE}{used} = '10' $b->{X-ANNOTATION-STORAGE}{used} = '0' at Cassandane/Cyrus/Quota.pm line 134 Cassandane::Cyrus::Quota::_check_usages('Cassandane::Cyrus::Quota=HASH(0x2dcdbc0)', 'storage', 8, 'message', 4, 'x-annotation-storage', 10) called at Cassandane/Cyrus/Quota.pm line 1018 Cassandane::Cyrus::Quota::test_upgrade_v2_4('Cassandane::Cyrus::Quota=HASH(0x2dcdbc0)') called at /usr/share/perl5/Test/Unit/TestCase.pm line 75 [...framework calls elided...]
Test was not successful.
brong
commented
12 years ago From: Greg Banks
Julien, are you planning to fix that last bug?
brong
commented
12 years ago From: Julien Coloos
Had a look at it. Proposed patch available as usual on our github repo: git://github.com/worldline-messaging/cyrus-imapd.git; branch ticket/3619
Pushed two commits:
57edc7b989051a4cdc3f7a7e48b10c7cd2a253f7 adds mailbox annotations size to mbexamine output This information can be useful :)
decc9b978e00d40aa0cfb7ee21b2c5a06e9e7be0 fixes index upgrade It's based on annotate_reconstruct API, which I renamed to recalc. Since the code was currently used for the reconstruct CLI, it could write things to stdout (when pruning orphan per-message annotations). I wondered whether to also do this pruning when upgrading the index, and finally let the action while disabling the stdout messages when not in reconstruct mode. If necessary, pruning could be disabled in annotate_recalc_commit. I also removed a few unused code lines in annotatemore commit/abort functions (quota_txn).
The test now passes, but I could not check other tests which almost all fails with the following message: Perl exception: Undefined subroutine &Cassandane::Generator::sha1_hex called at Cassandane/Generator.pm line 229 Cassandane::Generator::_generate_unique() called at Cassandane/Generator.pm line 274
Seems related to the recent changes to handle both Digest::SHA and Digest::SHA1 packages, but I did not yet find exactly why it fails here (FYI: perl v5.10.1).
brong
commented
12 years ago From: Julien Coloos
Found a fix for sha1_hex import. Available in our repo: git://github.com/worldline-messaging/cassandane.git; branch extra/misc Commit: fef45b06a94b1db44524a134a41884feefb8f958
My previous cyrus-imapd patch made another test (Bug3463) fail. The reason being that the quotaroot of the (unpacked) mailbox actually belonged to another (non-existent) mailbox. Compared to my previous code, the latest patch stops the index upgrade upon quota usage updating issues ... For the moment I reverted it to the previous behaviour in commit 8ac18aac008d09cdadc85c28ba04f2567f33a409 (cyrus-imapd repo), unless you think stopping upgrade in such cases is really right.
I also see 3 other test fails, even without the new code: 1) test_duplicate_suppression_on_uniqueid_delete(Cassandane::Cyrus::Delivery) Boolean assertion failed at /usr/local/share/perl/5.10.1/Test/Unit/Exception.pm line 13 Test::Unit::Exception::throw_new('Test::Unit::Failure=HASH(0xab5b748)', '-package', 'Cassandane::Cyrus::TestCase', '-file', 'Cassandane/Cyrus/TestCase.pm', '-line', 351, '-object', 'Cassandane::Cyrus::Delivery=HASH(0xab3f838)', ...) called at /usr/local/share/perl/5.10.1/Test/Unit/Assert.pm line 85 Test::Unit::Assert::do_assertion('Cassandane::Cyrus::Delivery=HASH(0xab3f838)', 'Test::Unit::Assertion::Boolean=SCALAR(0xab57f58)', 'Cassandane::Cyrus::TestCase', 'Cassandane/Cyrus/TestCase.pm', 351) called at /usr/local/share/perl/5.10.1/Test/Unit/Assert.pm line 19 Test::Unit::Assert::assert('Cassandane::Cyrus::Delivery=HASH(0xab3f838)', '') called at Cassandane/Cyrus/TestCase.pm line 351 Cassandane::Cyrus::TestCase::check_messages('Cassandane::Cyrus::Delivery=HASH(0xab3f838)', 'HASH(0xab4ec38)', 'check_guid', 0, 'keyed_on', 'uid') called at Cassandane/Cyrus/Delivery.pm line 278 Cassandane::Cyrus::Delivery::test_duplicate_suppression_on_uniqueid_delete('Cassandane::Cyrus::Delivery=HASH(0xab3f838)') called at /usr/local/share/perl/5.10.1/Test/Unit/TestCase.pm line 75 [...framework calls elided...]
2) test_uniqueid(Cassandane::Cyrus::Metadata) Boolean assertion failed at /usr/local/share/perl/5.10.1/Test/Unit/Exception.pm line 13 Test::Unit::Exception::throw_new('Test::Unit::Failure=HASH(0xae98128)', '-package', 'Cassandane::Cyrus::Metadata', '-file', 'Cassandane/Cyrus/Metadata.pm', '-line', 518, '-object', 'Cassandane::Cyrus::Metadata=HASH(0xae2fd68)', ...) called at /usr/local/share/perl/5.10.1/Test/Unit/Assert.pm line 85 Test::Unit::Assert::do_assertion('Cassandane::Cyrus::Metadata=HASH(0xae2fd68)', 'Test::Unit::Assertion::Boolean=SCALAR(0xae4fa08)', 'Cassandane::Cyrus::Metadata', 'Cassandane/Cyrus/Metadata.pm', 518) called at /usr/local/share/perl/5.10.1/Test/Unit/Assert.pm line 19 Test::Unit::Assert::assert('Cassandane::Cyrus::Metadata=HASH(0xae2fd68)', '') called at Cassandane/Cyrus/Metadata.pm line 518 Cassandane::Cyrus::Metadata::test_uniqueid('Cassandane::Cyrus::Metadata=HASH(0xae2fd68)') called at /usr/local/share/perl/5.10.1/Test/Unit/TestCase.pm line 75 [...framework calls elided...]
3) test_quota_f(Cassandane::Cyrus::Quota) expected 9, got 0 at Cassandane/Cyrus/Quota.pm line 885 Cassandane::Cyrus::Quota::test_quota_f('Cassandane::Cyrus::Quota=HASH(0xaf4f1a0)') called at /usr/local/share/perl/5.10.1/Test/Unit/TestCase.pm line 75 [...framework calls elided...]
Test was not successful.
Is it due to recent changes ? (I thought the only remaining test to fail was the quota upgrade).
PS: If you are interested, you will find a few other commits in our cassandane repo (which we are using when testing some of our specific stuff):
660a33ec0c30e2e7636fe2320b766dd613027656 Utility function to retrieve switch value out of configuration
3727e8c06a456d5ecb8190c099b62f3d08cde4be Necessary code for pop3d service with associated message store.
6bc0636163bce577dbf8b366dbbd3a2e58a7c4a8 When generating random data (which is mainly for messages), break it in lines.
ad2e6c2d8f22e3053bc25654a3fca1c11bff19ea Changes concerning the way target folder is handled when appending mails. The idea was to optimize a bit the action when dealing with lot of mails in one folder: previous code unconditionnaly close current folder to re-select it.
brong
commented
12 years ago From: Greg Banks
(In reply to comment #12) > Found a fix for sha1_hex import. Available in our repo: > git://github.com/worldline-messaging/cassandane.git; branch extra/misc > Commit: fef45b06a94b1db44524a134a41884feefb8f958
Thanks, that's very useful, pushed to cmu/master.
I've also pushed a bunch of stuff from my iris405 branch, in preparation for fixing up some of the problems noted by Jeroen, but I've left the Master tests disabled for the time being.
> My previous cyrus-imapd patch made another test (Bug3463) fail.
Two steps forward, one step back...
> The reason > being that the quotaroot of the (unpacked) mailbox actually belonged to another > (non-existent) mailbox. > Compared to my previous code, the latest patch stops the index upgrade upon > quota usage updating issues ... For the moment I reverted it to the previous > behaviour in commit 8ac18aac008d09cdadc85c28ba04f2567f33a409 (cyrus-imapd > repo), unless you think stopping upgrade in such cases is really right.
Not sure, have to think about it.
> I also see 3 other test fails, even without the new code:
I don't see those. With your latest code I see only one failure:
!!!FAILURES!!! Test Results: Run: 137, Failures: 1, Errors: 0
There was 1 failure: 1) test_quota_f(Cassandane::Cyrus::Quota) expected 9, got 0 at Cassandane/Cyrus/Quota.pm line 882 Cassandane::Cyrus::Quota::test_quota_f('Cassandane::Cyrus::Quota=HASH(0x3246188)') called at /usr/share/perl5/Test/Unit/TestCase.pm line 75 [...framework calls elided...]
Test was not successful.
I think I broke that one; but that test is need of some tweaking anyway.
> PS: If you are interested, you will find a few other commits in our cassandane > repo (which we are using when testing some of our specific stuff):
I haven't looked at these, remind me again after linux.conf.au (in about 2 weeks).
brong
commented
12 years ago From: Julien Coloos
> > I also see 3 other test fails, even without the new code: > > I don't see those. With your latest code I see only one failure: > ... > 1) test_quota_f(Cassandane::Cyrus::Quota) > expected 9, got 0 at Cassandane/Cyrus/Quota.pm line 882
It appears the two other failures I get are due to the fact I don't have libuuid on my system. From what I could see, when generating a new folder unique id cyrus will use libuuid if present, or combine a hash of the folder name and the folder uidvalidity (which happens to be the creation time, precision of 1s). Both tests do use the same folder name twice (created, deleted, re-created). So here each creation ends up with the same unique id (done in the same second), which triggers test failures:
I am not sure there are noticeable chances to have this scenario in real life, but maybe the 'default' (libuuid not used) unique id could be added something to take care of this. Or maybe libuuid should be documented as highly recommended (or even made mandatory) ?
brong
commented
12 years ago From: Greg Banks
(In reply to comment #14) > > It appears the two other failures I get are due to the fact I don't have > libuuid on my system.
Really!? Wow. On my system /sbin/mkfs.ext3 uses libuuid1, so it's pretty much a required package.
> From what I could see, when generating a new folder unique id cyrus will use > libuuid if present,
Yes, since Bug 3601.
> Both tests do use the same folder name twice (created, deleted, re-created). So > here each creation ends up with the same unique id (done in the same second), > which triggers test failures: > - one test is checking the second unique id is different from the first > - the other test prunes delivery duplicates, which I guess are based on the > unique id of the folder and some properties of the message
Yes.
> If I manually delays (1s) the folder re-creation, tests do pass (since > uidvality, and thus unique id, is then different).
Sure. Except, the tests are specifically designed to test for that broken behaviour, so please don't do that.
> I am not sure there are noticeable chances to have this scenario in real life,
We have actually seen it happen at fastmail, I believe once or twice. It is rare.
> but maybe the 'default' (libuuid not used) unique id could be added something > to take care of this. Or maybe libuuid should be documented as highly > recommended (or even made mandatory) ?
We could make libuuid mandatory in the configure script.
brong
commented
12 years ago From: Greg Banks
I finally have Cassandane fully passing against cmu/master, even under Jenkins.
brong
commented
12 years ago From: Greg Banks
So the latest Cassandane 'master' branch passes all tests with the latest Cyrus 'master' branch, and is doing so twice a day at ci.cyrusimap.org.
elliefm
commented
7 years ago See #1
From: Bron Gondwana Bugzilla-Id: 3619 Version: unspecified Owner: Greg Banks