Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Integration testing: list of ideas of improvement #1465

Closed
mattab opened this issue Jul 6, 2010 · 30 comments
Closed

Integration testing: list of ideas of improvement #1465

mattab opened this issue Jul 6, 2010 · 30 comments
Labels
Enhancement For new feature suggestions that enhance Matomo's capabilities or add a new report, new API etc. Major Indicates the severity or impact or benefit of an issue is much higher than normal but not critical.

Comments

@mattab
Copy link
Member

mattab commented Jul 6, 2010

This ticket is a placeholder for improvements related to integration tests and tests in general.

Check out existing tests (Main.test.php or Integration.test.php)

Improve tests and increase coverage:

  • Widgets HTML output should be tested against an expected HTML output. See QA: Enable Widgets HTML compare integration tests #2908
  • Add tests for the standard API filter parameters (filter_*)
  • Add a test which fetches a given subtable (equivalent to clicking on a row to fetch the subrow)
  • Add a test using API authentication with token_auth
  • Some modules lack unit tests, we could add them:
  • Goal API add/update/delete,
  • LanguagesManager,
  • SEO.getRank,
  • Test that generated PDF, HTML & SMS reports, similarly to generated XML, don't change when not expected to. See Include scheduled reports in integration tests #3323

See also related ticket and comments for more ideas: #1470

@mattab
Copy link
Member Author

mattab commented Aug 6, 2010

(In [2877]) Refs #1465 Adding a file that loads and displays in the browser all widgets that are registered in the Piwik install.
This could ideally be loaded by Webtests to ensure that

  • there is no JS error
  • all widgets display correctly with data as expected

@mattab
Copy link
Member Author

mattab commented Nov 14, 2010

Random notes: Tracking algorithm could test that

  • negative id site should not throw exception
  • a new visitor starts the visit by clicking on an outlink on one of the alias host for the website. Is this considered a new visit, or should it be ignored?

@mattab
Copy link
Member Author

mattab commented Dec 22, 2010

Moving "Tracker should set cookies in the request and have a different IP at each request. Test that unique visitors counts is correct." to #1912

@mattab
Copy link
Member Author

mattab commented Jan 20, 2011

Are Javascript tests run in Hudson at the moment? I assume they are not, since Webtest doesn't have a proper browser?

@robocoder
Copy link
Contributor

Correct, not running in Hudson. (This is mentioned in #1470.) I'll be looking into using Selenium.

@diosmosis
Copy link
Member

I've got an idea on how to implement the integration tests for widgets/Controllers. The integration tests could be split up into smaller files similar to:


class OneVisitorTwoVisitsTest extends IntegrationTestBase
{
    public function getApiToTest()
    {
        return array(
            'Goals.this',
            'Actions.that' => array(
                'setDateLastN' => true,
                // other options
            )
        );
    }

    public function getControllersToTest()
    {
        return array('Goals.get', 'Actions.get');
    }

    protected function setUp()
    {
        parent::setUp();

        // add website & visits...
    }

    protected function doTest()
    {
        // do extra tests (testing stuff other than API/Controllers)
    }
}

IntegrationTestBase (or whatever it'll be called) will define a test_main() method that runs tests based on the results of getApiToTest() & getControllersToTest().

I think it could also be possible to simply add a method similar to the callGetApiCompareOutput method, but this would result in less readable code.

Sound like a good idea?

@mattab
Copy link
Member Author

mattab commented Dec 28, 2011

Refactoring Main.test.php in several files would be a good idea for sure.

But, maybe this is not required for this feature. For example, the same way we call callGetApiCompareOutput() we could callWidgetsCompareOutput() which would load a given set of widgets (or load ALL widgets) and compare the HTML output with the pre-recorded output.

You are welcome to do the refactoring but I wanted to point out the "Direct" way as well ;)

@diosmosis
Copy link
Member

I thought of the 'direct' way too, but I thought I could kill two birds w/ one stone.

@diosmosis
Copy link
Member

(In [5666]) Refs #1465. Added ability to test controller actions to integration tests. Added two controller tests, one that tests MultiSites.index & another that tests all testable controller methods.

Notes:

  • Fixed PHP_Notices that were generated by the UserSettings plugin.
  • Controller action testing behavior can be changed by setting the widget testing level. At the moment, the default level is set to CHECK_WIDGET_ERRORS which only checks for recoverable PHP errors. It can be set to skip widget testing or compare the widget output w/ expected files as well.
  • Widget testing can be done at every access level ('anonymous', 'view', 'admin', 'superuser').
  • Not all controller actions can be tested, those that can't (at the moment) are blacklisted from being tested when 'all' actions are tested.

@diosmosis
Copy link
Member

(In [5667]) Refs #1465. Fixing the build (hopefully).

@diosmosis
Copy link
Member

(In [5668]) Refs #1465. Refactored integration tests by splitting each individual test in Main.test.php up into separate files.

NOTES:

  • New tests use the Test_Integration_Facade type that makes the tests easier to read.
  • Split Database.test.php into two types (in the same file) so the hello world test cases aren't duplicated every time another type derives from it.
  • Number of passes changed from 10233 to 10278 for all tests. The increase is due to further splitting up of certain tests.

@mattab
Copy link
Member Author

mattab commented Jan 26, 2012

capedfuzz, thanks for the improvements and contributions!

Code review:

** Controller action testing behavior can be changed by setting the widget testing level. At the moment, the default level is set to CHECK_WIDGET_ERRORS which only checks for recoverable PHP errors. It can be set to skip widget testing or compare the widget output w/ expected files as well.

  • I think this is a good start, but can we enable COMPARE_WIDGET_OUTPUT mode. It would be very useful to spot when widgets output change and we do not expect it.
    • The .html outputs of all widgets tested would be stored in a directory the same way the API outputs, for example in tests/integration/expected-widgets/Referers.getKeywords.html
    • The controller "test all methods" would be called for only 1 test maybe, since it will be slow to run, we need only run it on one test case
  • tearDown()
    // add access to all test users if doing controller tests
    Why do you add access in the teardown? tear down should delete all created users and access, and setUp() would create users + access
  • Great work on the Facade refactoring. It makes the test a lot more readable and easier to manage.
    • Can you please confirm that you have carefully copy paste all tests and haven't had to do any modification to the code that could/should have impacted the tests?
  • How to replicate the old Main.test.php behavior, and execute all integration tests (now split in different files) automatically?

@diosmosis
Copy link
Member

Replying to matt:

  • I think this is a good start, but can we enable COMPARE_WIDGET_OUTPUT mode. It would be very useful to spot when widgets output change and we do not expect it.
    • The .html outputs of all widgets tested would be stored in a directory the same way the API outputs, for example in tests/integration/expected-widgets/Referers.getKeywords.html
    • The controller "test all methods" would be called for only 1 test maybe, since it will be slow to run, we need only run it on one test case

Right now I added a test that tested all methods, but doesn't compare the widget output (its in the OneVisitorTwoVisits test). This could be changed, but that would mean looking through all the controller output, which I didn't have time for. If you think it'd be ok to assume what's generated right now is correct, I can make the change right now.

  • tearDown()
    // add access to all test users if doing controller tests
    Why do you add access in the teardown? tear down should delete all created users and access, and setUp() would create users + access

That code isn't in tearDown, it's in the createWebsite function.

  • Great work on the Facade refactoring. It makes the test a lot more readable and easier to manage.
    • Can you please confirm that you have carefully copy paste all tests and haven't had to do any modification to the code that could/should have impacted the tests?

It's been a bit since I made the changes, so I'll have to look it over again, but I remember only making a few changes to the CsvExport tests (regarding changing the language). Also, the tests all passed w/o modification and I was able to explain the increase in passes. I'll look through the commit to double check, though.

  • How to replicate the old Main.test.php behavior, and execute all integration tests (now split in different files) automatically?

I guess I could add a test group for this? Or maybe just a file like all_tests.php that only looks for integration tests? A little while ago I wrote a small alternative to all_tests.php that ran each test via AJAX so you would be able to run tests individually and see each test's progress after it finished. I was going to email it in case it was useful, but before I do, I could add a way to run all integration tests.

@mattab
Copy link
Member Author

mattab commented Jan 31, 2012

Right now I added a test that tested all methods, but doesn't compare the widget output (its in the OneVisitorTwoVisits test). This could be changed, but that would mean looking through all the controller output, which I didn't have time for. If you think it'd be ok to assume what's generated right now is correct, I can make the change right now.

Please enable this feature for now (I will disable later if need be).

You can copy all the widgets output as long as they seem to work and return data.

I guess I could add a test group for this? Or maybe just a file like all_tests.php that only looks for integration tests?

Perfect idea to add a script to trigger only integration tests.

However, if you also enable HTML Widget comparisons, would it make sense to have run_integration_tests script to run either Widget HTML compare, API html compare, or both (which would be the default mode)?

Thanks!!

@mattab
Copy link
Member Author

mattab commented Feb 2, 2012

(In [5739]) * Refs #534 Removing exit_rate from metadata API, it was added in the standard processed metrics causing other reports to display it with 0% values

@mattab
Copy link
Member Author

mattab commented Feb 2, 2012

(In [5743]) Refs #1465
Kuddos cappedfuzz for the great refactoring!

Fixes bug that a subset of the previous tests were ignored.
FYI: To find out these missing tests, I deleted all files in processed/ directory, then ran all_tests.php - I then noticed maybe 40 files or so, that were in expected/ but not in processed/ meaning they were not generated but they should have been.

@diosmosis
Copy link
Member

(In [5747]) Refs #1465. Added all_integration_tests.php script that invokes all integration tests and nothing else, and refactored all_tests.php. Also added the ability to disable API integration tests & modify the widget testing level via the browser.

NOTES:

  • Removed widget tests for ExampleRssWidget since it depends on a feedburner URL.

@diosmosis
Copy link
Member

Just committed a change that allows you to execute all integration tests, but nothing else. Also, its possible to disable API testing & modify the widget testing level by setting the apiTestingLevel & widgetTestingLevel query params. So,

/tests/all_integration_tests.php?apiTestingLevel=none&widgetTestingLevel=compare_output

will skip the API tests & compare widget output. These are the possible values for both params:

  • apiTestingLevel: 'none' or 'compare_output'
  • widgetTestingLevel: 'none' or 'compare_output' or 'check_errors'

@mattab
Copy link
Member Author

mattab commented Feb 3, 2012

(In [5752]) Refs #1465

  • Added tests/indexs.php as a welcoming page for piwik devs, to link to the most commonly used tests
  • Linked to the new capabilities of testing Widget for errors and/or compare HTML
  • slightly refactored/renamed

@diosmosis
Copy link
Member

(In [5764]) Refs #1465, disabled widget comparison in order to get jenkins build to pass. Can't reproduce the errors jenkins gets.

@robocoder
Copy link
Contributor

After r5764, I "downgraded" the CI server from PHP5.4.0RC5 to PHP5.3.10 since that's a security release and needs to be tested. Not sure if that's coincidental with the build currently failing though.

@robocoder
Copy link
Contributor

(In [5768]) refs #1465 - comment out output buffering; appears to be a side effect of r5752

[Sun Feb 05 10:44:17 2012] [warn] [client 127.0.0.1] Timeout waiting for output from CGI script /home/www/bin/php-cgi, referer: http://localhost/piwik/tests/all_integration_tests.php
[Sun Feb 05 10:44:17 2012] [error] [client 127.0.0.1] (70007)The timeout specified has expired: ap_content_length_filter: apr_bucket_read() failed, referer: http://localhost/piwik/tests/all_integration_tests.php

@robocoder
Copy link
Contributor

The CI build is currently "passing" with php5.3.10 (after reverting r5703) but this might be a false positive because I'm seeing what looks like an error in the build's console log. Will need to diagnose that before testing on php5.4.0rc7.

@robocoder
Copy link
Contributor

(In [5770]) refs #1465 - revert r5768; add paintMethodEnd() to flush buffers intermittently

@mattab
Copy link
Member Author

mattab commented Feb 8, 2012

Widget HTML compare feature moved to its own ticket: #2908

@mattab
Copy link
Member Author

mattab commented Feb 8, 2012

(In [5783]) Refs #1465 Trying to remove the message reported by Anthon in http://qa.piwik.org:8080/jenkins/job/Piwik/2837/consoleFull

@mattab
Copy link
Member Author

mattab commented Feb 8, 2012

(In [5785]) Refs #1465 Completely disabling widget testing since it fails for mysqli for some unknown reasons (to be investigated later)
Refs #2908

eg. See log in http://qa.piwik.org:8080/jenkins/job/Piwik/2838/consoleFull

@mattab
Copy link
Member Author

mattab commented Feb 8, 2012

@vipsoft Tests now pass on mysqli without the message after disabling widget output compare

@julienmoumne
Copy link
Member

adding reference to #3323

@mattab
Copy link
Member Author

mattab commented Dec 14, 2012

all these items are now done IMHO

The only remaining idea is to enable integration test for widgets covered in #2908

@mattab mattab added this to the Future releases milestone Jul 8, 2014
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement For new feature suggestions that enhance Matomo's capabilities or add a new report, new API etc. Major Indicates the severity or impact or benefit of an issue is much higher than normal but not critical.
Projects
None yet
Development

No branches or pull requests

4 participants