Opened 4 years ago

Closed 17 months ago

#1465 closed New feature (fixed)

Integration testing: list of ideas of improvement

Reported by: matt Owned by:
Priority: major Milestone: Future releases
Component: Core Keywords:
Cc: Sensitive: no

Description (last modified by JulienM)

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 #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 #3323

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

Change History (35)

comment:1 Changed 4 years ago by matt (mattab)

(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

comment:2 Changed 4 years ago by matt (mattab)

  • Description modified (diff)

comment:3 Changed 3 years ago by matt (mattab)

  • Description modified (diff)

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?

comment:4 Changed 3 years ago by matt (mattab)

  • Description modified (diff)

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

comment:5 Changed 3 years ago by matt (mattab)

  • Description modified (diff)

comment:6 Changed 3 years ago by matt (mattab)

  • Description modified (diff)

comment:7 Changed 3 years ago by matt (mattab)

  • Description modified (diff)

comment:8 Changed 3 years ago by matt (mattab)

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

comment:9 Changed 3 years ago by vipsoft (robocoder)

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

comment:10 Changed 2 years ago by matt (mattab)

  • Description modified (diff)

comment:11 Changed 2 years ago by capedfuzz (diosmosis)

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?

comment:12 follow-up: Changed 2 years ago by matt (mattab)

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 ;)

comment:13 in reply to: ↑ 12 Changed 2 years ago by capedfuzz (diosmosis)

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

comment:14 Changed 2 years ago by capedfuzz (diosmosis)

(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.

comment:15 Changed 2 years ago by capedfuzz (diosmosis)

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

comment:16 Changed 2 years ago by capedfuzz (diosmosis)

(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.

comment:17 follow-up: Changed 2 years ago by matt (mattab)

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_?

comment:18 in reply to: ↑ 17 Changed 2 years ago by capedfuzz (diosmosis)

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.

comment:19 Changed 2 years ago by matt (mattab)

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!!

comment:20 Changed 2 years ago by matt (mattab)

(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

  • Refs #1465 Leave underscores in test names just to keep tests more readable (even though it is not a best practise to have _ in class prefixes!!)

comment:21 Changed 2 years ago by matt (mattab)

(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.

comment:22 Changed 2 years ago by capedfuzz (diosmosis)

(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.

comment:23 Changed 2 years ago by capedfuzz (diosmosis)

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'

comment:24 Changed 2 years ago by matt (mattab)

(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

comment:25 Changed 2 years ago by capedfuzz (diosmosis)

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

comment:26 Changed 2 years ago by vipsoft (robocoder)

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.

comment:27 Changed 2 years ago by vipsoft (robocoder)

(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

comment:28 Changed 2 years ago by vipsoft (robocoder)

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.

comment:29 Changed 2 years ago by vipsoft (robocoder)

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

comment:30 Changed 2 years ago by matt (mattab)

  • Description modified (diff)

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

comment:31 Changed 2 years ago by matt (mattab)

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

comment:32 Changed 2 years ago by matt (mattab)

(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

comment:33 Changed 2 years ago by matt (mattab)

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

comment:34 Changed 21 months ago by JulienM (JulienMoumne)

  • Description modified (diff)

adding reference to #3323

comment:35 Changed 17 months ago by matt (mattab)

  • Resolution set to fixed
  • Status changed from new to closed

all these items are now done IMHO

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

Note: See TracTickets for help on using tickets.