Feature Proposal: Move to a single Unit Test suite that runs on multiple branches.

Motivation

Currently our unit tests suite tend to diverge, resulting in inconsistency between trunk and release branch. Several situations occur:

• Over time unit tests get added to the release branch without being synced to trunk, which leads to fixes being missed on trunk.
• Tests are added to trunk for new features (that are not expected to pass on the release branch)
• Tests are added to trunk for existing features (often to help prevent breakage during trunk only work) that should pass on the release branch.
• Bug fixes on both trunk and release branch are occasionally missed, exposing us to re-breakage in the "next release".
• Manually keeping the unit tests in sync between branches is a continual effort and is often neglected.

The above situations have all occurred, and in some cases have resulted in major inconsistency between releases that were later detected by users, requiring patch releases.

Changing to a unified test suite should have considerable benefits

• Clearly annotates differences between trunk and release branches
• Minimize opportunity to miss branch fixes on trunk, or trunk bug fixes on the branches.
• Eliminate the effort to keep the branch Unit Tests in sync with trunk

Description and Documentation

Enhance the Unit Test framework to allow for:

• Skipping of tests for features not implemented on an older release
• Conditional assertions within a test when divergence between release is expected.

Changes to development release process:

• Manually perform a one-time sync-up between the release01x01 and trunk.
• Once 01x02 branches from trunk, the UnitTestContrib will not be branched. (The git based auto-install of extensions during pseudo-install will allow testing with the trunk version on release01x02.)

This feature request is for work that has mostly been already completed. PaulHarvey has created the changes to the Unit Test framework, and also has performed a complete sync-up of the trunk and Release01x01 UnitTestContrib. (Multiple times)

The proposal is to gain consensus that the UnitTestContrib should not be branched for 1.2, and to get general acceptance that the current 1.1 version should remain in sync with trunk.

Examples

From the documentation in test/unit/Foswiki/FoswikiTestCase.pm

ObjectMethod skip_test_if($test, @skip_data) -> $reason

Skip $test if it is listed under a satisified condition key in %skip_data,
return $reason string if the test should be skipped, undef otherwise.

• $test - name of the test under consideration
• @skip_data- array of hashrefs, containing two keys:
  • condition - value is a hashref understood by check_conditions_met
  • tests - $test => $reason key/value pairs.

Example:

sub skip {
    my ( $this, $test ) = @_;

    return $this->SUPER::skip_test_if(
        $test,
        {
            # This condition matches on Foswiki versions < 1.2
            condition => { with_dep => 'Foswiki,<,1.2' },
            tests     => {
                # All permutations of verify_TableParser are skipped
                'TestSuite::verify_TableParser' =>
                  'TableParser not implemented until Foswiki 1.2',
            }
        },
        {
            # This condition matches on Foswikis 1.2+ with ShortURL config
            condition => {
                with_dep => 'Foswiki,>=,1.2',
                using    => 'ShortURLs',
            },
            tests => {
                # Only this permutation of verify_request is skipped
                'TestSuite::verify_request_redirect' =>
                  'This test makes no sense on Foswiki 1.2+ w/ShortURLs',
            },
        },
        {
            # This condition matches when Webservice::Solr is missing
            condition => { without_dep => 'Webservice::Solr' },
            tests     => {
                # Example of skipping an individual test
                'TestSuite::test_solr_thing' => 'Solr perl library missing',
            },
        }
    );
}

ObjectMethod check_dependency($what) -> $boolean

Checks to see if a given dependency is present, optionally of a specified version

This is a wrapper to Foswiki::Configure::Dependency->check()

• $what - a string (or arrayref of strings) specifying module(s) to check for, optionally of specific version(s). The string(s) should be compatible with BuildContrib 's DEPENDENCIES file. When an arrayref is passed, returns true if all dependencies are met. Therefore check_dependency(['a', 'b']) is logically equivalent to check_dependency('a') && check_dependency('b')

Examples:

$this->check_dependency('JSON');
$this->check_dependency('CGI,=,3.43'),
$this->check_dependency(['CGI,=,3.43','Foswiki,<,2.0']),

Use this to check different conditions, for example:
    if ( $this->check_dependency('Foswiki,>=,1.2') ) {
        assert for something only applicable to 1.2
    }

ObjectMethod expect_failure([$reason,] [%conditions])

Flag that the test is expected to fail in the current environment. This is used for example on platfroms where tests are known to fail e.g. case sensitivity of filenames on Win32.

• $reason - Optional. String with reason for why failure is expected.
• %conditions - Optional. Conditions to be met for failure to be expected see check_conditions_met()

This proposal added the %conditions option

Examples:

$this->expect_failure();
$this->expect_failure('Feature not yet implemented');
$this->expect_failure( using => 'ShortURLs' );
$this->expect_failure( with_dep => 'Foswiki,<,1.1' );
$this->expect_failure(
    'Requires ADDTOZONE feature',
    not_using   => 'ZonePlugin'
    with_dep => 'Foswiki,<,1.1'
);
$this->expect_failure(
    'Javascript and perl/Foswiki have different ideas about true & false',
    using => 'MongoDBPlugin'
);
$this->expect_failure(
    'Can\'t grep on windows',
    using => ['PlatformWindows', 'SearchAlgorithmForking']
);
$this->expect_failure(
    'CGI.pm 3.43 causes double-encoding when using utf-8',
    using => 'unicode',
    with_dep => 'CGI,=,3.43'
);

ObjectMethod check_conditions_met(%conditions) -> $boolean

Check that ALL %conditions are met in the environment under test.

• %conditions- Hash of conditions to check. All must be met. Keys:
  • using - String (or arrayref of strings) of named configuration profile(s), feature(s) or plugin(s) which must be in use for the failure to be expected. See check_using()
  • not_using - As with using, but inverted sense (expect failure when NOT using feature(s)/config(s)/plugin(s))
  • with_dep - String (or arrayref of strings) of module(s), optionally of specific version(s) which must be present for the failure to be expected. Same strings as each line in BuildContrib 's DEPENDENCIES. See check_dependency()
  • without_dep - opposite of with_dep

Impact

• Reduces effort needed to keep unit tests in sync
• Increases code complexity of tests that are different between branches.
• Improves odds of finding breakage between branches

Developer impact

• Until 1.2 is branched, please continue to keep 1.1 unit tests synchronized with trunk.
• Tests for new features that are not expected to pass on older branches should be completely skipped.
  • Add a sub skip to set the conditions for skipping the tests. See example in test/unit/QueryTests.pm
• Tests that are expected to fail on an older release (ex. a bug that is not expected to be fixed).
  • Annotate with expect failure, preferably ordering the asserts so that expected failures are last in the test.
  • Any assert that fails prior to the expect_failure call will generate a true failure..
  • Any assert that succeeds after the expect_failure will cause an unexpected pass.

$this->expect_failure( 'Item11708 Store API fixed in Foswiki 1.2+',
        with_dep => 'Foswiki,<,1.2' );

WhatDoesItAffect:

%WHATDOESITAFFECT%

edit

Implementation

Tasks.Item11479 implemented the enhancements and applied the conditions to the tests, so that the trunk tests would pass successfully on 1.1 branch.

Tasks.Item11956 reorganized the reporting of failures.

-- Contributors: GeorgeClark - 22 Aug 2012

Discussion

While I think this is a worthy feature, let me say that it does make writing Foswiki tests even more complex for new people.

Not letting trunk diverge too much from the last release, that is: release from trunk more often, would be another way to achieve this, no? In my impression, major new features are already developed in git branches away from trunk, so why not make it a habit to keep trunk in a releasable state, release from trunk at least once a year, and limit patch releases to urgent bugfixes?

-- FlorianSchlichting - 23 Aug 2012

I have to agree with Florian.

-- MichaelDaum - 23 Aug 2012

I don't see how this actually makes it more complex for the new developer. And it probably applies to new core developers, not plugins. Consider the following:

Fix a small bug on Release branch

You modify or write a test to expose the bug, and add it to the only UnitTestContrib. The nightly tests will also flag that the same bug exists in other branches. Fix it there and you are done. Forgetting to commit to multiple branches is a common issue.

Fix a bug in trunk

If it didn't exist in the release branch, the test passes there as well, If it doesn't pass, but the release is not being maintained, either ignore it, or cut/paste in an expect failure message with a comment that the bug won't be fixed in that release.

Add a new feature

Test fails on other releases. Add the "skip" condition.

I expect that if a new developer were adding tests, nobody will complain if they are failing on other releases. It's not much effort to add the conditionals if the failure is expected. And it flags omissions and incompatibilities which will make foswiki more reliable.

If you look over the UnitTestContrib, As of a month or so ago, the trunk tests would pass on the release branch.

• There are 7 tests that use the check_dependency test to modify the test based upon the release
• There are 12 instances of skip sections.
• And a total of 46 expect failures. (And they are mostly written for missing dependencies, such as the password tests that can't work if the cpan hash module is missing)

The majority of the time the tests do just run on multiple releases. So finding out when they don't is important. I'd much rather have a failing test to review than no test at all.

-- GeorgeClark - 23 Aug 2012

Maybe I would be more convinced seeing an example unit test to get a feeling for the extra complexity...

-- MichaelDaum - 23 Aug 2012

These have actually been in our unit tests starting around last January. There are quite a few examples:

• The AddressTests as a whole don't apply to pre 1.2, as there is no Address object

sub skip {
    my ( $this, $test ) = @_;

    return $this->check_dependency('Foswiki,<,1.2')
      ? 'Foswiki 1.1 has no Foswiki::Address'
      : undef;
}

• The FuncTests for the ForceDefaultUrlHost feature is a 1.2 version only, so skip that test. (The hash array has the test to be skipped on the left side => Message reported to the test run log on the right)

sub skip {
    my ( $this, $test ) = @_;

    return $this->SUPER::skip_test_if(
        $test,
        {
            condition => { with_dep => 'Foswiki,<,1.2' },
            tests     => {
                'FuncTests::test_getUrlHost_ForceDefaultUrlHost' =>
                  'ForceDefaultUrlHost is Foswiki 1.2+ only, Item11900',
            }
        }
    );
}

• One CrawfordCurrie just added to the FormattingTests - a small tweak to rendered content..

+        if ( $this->check_dependency('Foswiki,<,1.2') ) {
+
+            # URL encoding of whitespace was borked before 1.2
+            $expected =~ s/%0a/%20/g;
+            $expected =~ s/%([A-Za-z0-9]{2})/%\U$1\E/g;
+        }

• Look for the sub skip in PasswordTests.pm. In this case, it's not backwards compatibility. Certain tests are skipped if dependencies are missing.

• In the StoreImplementationTests, an assert will fail on releases before 1.2.

    $this->expect_failure( 'Item11708 Store API fixed in Foswiki 1.2+',
        with_dep => 'Foswiki,<,1.2' );
    $this->assert_num_equals( 1, $info->{version} );

-- GeorgeClark - 23 Aug 2012

This is complicated.

-- MichaelDaum - 23 Aug 2012

It's complicated, but I can understand the motivation. Maintaining compatibility is important to most people, and we rely on the unit tests to help us do that. If we allow the test sets to diverge, we are not comparing oranges with oranges.

Having said that, I really don't like peppering the tests with conditionals. I can only see a scenario where the number of conditions rises, and never falls.

-- CrawfordCurrie - 24 Aug 2012

The complexity not only comes from adding spaghetti to the code but also due to the fact that maintaining the tests needs all covered Foswiki versions to be installed and available for testing. Otherwise you won't know whether the conditionals are correct or not. That won't happen for most of us, I guess. So I suspect we will still end up with unit tests being written and checked against one engine, remaining unsure what the other engines will say on the same bunch now that they have been changed.

I'd prefer a release + unit tests not to be touched once approved.

The situation of syncing development on two branches, stable and trunk, will always remain tricky to have bugs fixed on both ends. I am not sure whether unifying the test suites help or do more harm to the situation. When a bug got fixed on stable but not on trunk, then this is a disaster no matter what, much less than the other way around.

-- MichaelDaum - 24 Aug 2012

I don't believe that there should be any attempt to maintain more than two supported releases. Once a release is "stabilized" like 1.0.10, at that point we copy over UTC into that branch and stop maintaining the backwatds compatibility. We should use this to stop the double maintenance on the two active develooment streams ongoing at any time.

I doubt that there would be much effort to purge out conditions for the obsolete versions, but certainly if you find you are adding a 3rd condition it's time to remove the obsolete code.

The objective is to stop the need to double-commit each test change, not maintain a test suite that will run on every release ever made.

And as far as needing to test on every release, hopefully that gets handled by the "nightly" test runs. So you write the test for trunk or release, and the nightlies will complain if there is an incompatibility. At least that way we find out that something changed that may or may not be desirable.

-- GeorgeClark - 24 Aug 2012

Thanks George for creating this proposal in my absence, and bringing it to my attention.

• This should only impact a very few tests which require different behaviour on different branches.
• If you want to ignore these new mechanisms, you can continue to do so. It should create absolutely no extra work.
• I did invite feedback at the time, on IRC and on foswiki-svn. I will admit that the skip stuff is ugly, but I think the rest of it is passable. Please do offer suggestions for improvement.
• I didn't do a proposal because I didn't intend for this to be a big deal. And I think I was right: the work was done back in January, and it's only now that people are noticing.

This effort was inspired by the task of trying to make it feasible to keep the work in unicode and store2 branches on github in a state which could be re-integrated back into Foswiki SVN.

Maintaining four different branches of our test suite is just too much. Even just unifying our trunk & release branches brings many benefits.

check_using, check_dependency were inspired by just pursuing Don't Repeat Yourself (DRY) principle. We shouldn't have to copy-paste the same 10-15 lines again and again just to test Eg. module versions, are-we-on-windows, which-rcs, etc.

The extension to expect_failure I don't think is a big deal. It's just a few extra parameters.

The skip_if stuff is ugly, but seldom used. Suggestions welcome.

-- PaulHarvey - 11 Sep 2012

Moving this to Accepted Proposal. Implementation will be that when we branch 1.2, we won't branch UnitTestContrib. Should we remove UnitTestContrib from 1.1 as well?

-- GeorgeClark - 09 Oct 2012

Note that by running the trunk test suite on 1.1. I've been able to discover a fairly serious issue with Func::checkAccessPermissions. It was fixed as part of the major store changes in trunk, but was not really related to the store performance issues. Created Tasks.Item12198 to pick up the fixes on 1.1.

-- GeorgeClark - 28 Oct 2012

I think it makes sense to tag UnitTestContrib rather than branch it.

If we create tags alongside the releases, there should be no need to keep the Release01x01 branch hanging around.

-- PaulHarvey - 30 Oct 2012

Commenting is disabled while not logged in