Tests for update.module

whoops.

Change component is relation to http://drupal.org/node/253744.

This was one of the modules that we weren't planning on witting tests for unless someone comes up with a way to.

Related: http://drupal.org/node/243096

The best way to make tests for this would be to either harvest or manually construct some of the XML input files that update.module fetches and include those with the tests. Then, you could write the tests such that they force update.module to fetch your local copies of the input files, and to alter the .info for the core modules to do a "parameter sweep" for the various states of not-fully-up-to-date that a site could be in. Each test would setup an initial state and "current" state of available releases, then compare the output of various functions relative to what you'd expect.

That said, there are some pretty major API refactorings that should happen to make this module easier to test, which would also help solve problems such as #232041: Fatal error: Maximum execution time of 60 seconds exceeded (when fetching update status data) and #238950: Reduce RAM resource consumption.

Looks like this is waiting on API patches.

Repairing the component now that we're no longer using the "tests" component (yay).

We could set the default project server variable to the testing server itself and already start to test the rough functionality. Thus, marking as active.

Here are some update tests, including mock XML and a test module in a /tests/ folder.

Seems to be missing update.test :)

Sorry, here it is.

Chx caught a stray file_put_contents(), removed now.

Good to see this get done!

Technically won't matter? but should be $Id$.

Perhaps use readfile(), http://us2.php.net/manual/en/function.readfile.php.

15 days to code freeze. Better review yourself.

Rerolled to fix a minor problem -- the tags for these official releases didn't match what they'd really be. Doesn't actually impact the tests, but we should be as accurate as possible here in the sample input data.

I wish I had time to more carefully play with these and verify they're testing the right sorts of things, but sadly I don't have time for that this afternoon (and probably not this week). This is a *far* cry from testing everything that needs to be tested, but it's certainly a good start if it works. ;) webchick/dries certainly have my blessings to commit without a more careful review from me -- we can always clean this up and expand it later.

Re: #13: CVS will fix $Id$ automatically -- doesn't matter what it is in the patch.

The last submitted patch failed testing.

Just tried testing again. Patch applies cleanly and all the new tests run fine. I think it was marked 'needs work' previously at a time when HEAD was broken. However, rerolled to use readfile(), and fixed the CVS $Id$ stuff.

This is clearly better than 0 test coverage for update.module. There's still much more to add, but we can do that in future issues/patches. Meanwhile, if we get this in a) we have something and b) we already have the file and directory structure we need to add more tests in other issues.

Apparently we no longer use $items = array(); syntax in menu hooks, and go straight to adding items to the array, so this needs to be rerolled for that. Otherwise it looks very good - unless we want to make the indentation on the xml files nicer? That's probably not necessary though.

@cwgordon: is there an issue link for this hook_menu() change? Whatever that patch was didn't fix update_menu() either...

Anyway, rerolled for that. *shrug*

Note to committers, after applying this patch, but before committing, don't forget:

I guess there might not be a case for the = array()/!=array(), I don't know, but it shouldn't really matter either way. The patch looks good, and should be RTBC.

Better still. The act of writing a test for #499828: Wrong release date on multi-module projects showed me that we need update_test.module to be more flexible. Instead of hard-coding an initial state of the core .info files (via hook_system_info_alter()), we want to specify that initial state via a variable, so that each test case can define its own initial state however it needs.

If we running this line on every test in this test case, why not add it to setUp()?

This review is powered by Dreditor.

Nevermind...the test meant for #499828: Wrong release date on multi-module projects would not be using it, so it wouldn't make sense to put it in setUp() now.

That'll probably be replaced properly when committed to CVS...I think. Everything looks good and great, great job using the pluggable architecture of the update system to get started on tests. Bravo.

This review is powered by Dreditor.

Committed to CVS HEAD. Yay for tests!

I'm glad this is here and I appreciate all the work it must have been to build these.

I'm all for tests, but I looked at this since it is causing:

#538660: Move update manager upgrade process into new authorize.php file (and make it actually work) to fail.

Why is it failing? Because the url changed from admin/reports/update to admin/reports/update/full as the simplified view for PM will be the DEFAULT_LOCAL_TASK.

I know that this is not the only instance in Drupal core of a test which is pretending to be a browser and looking for some HTML to show up. But is there anyway we can make this test test a unit of code? As it is, it doesn't allow for changes like unit tests are supposed to, it does the opposite!

By just seeing that some drupal message is set, or some title comes up we are:

a). Not testing the code in question but the entire stack.
b). Making a very brittle test (case in point), where if a URL changes, or the markup around the text we want changes, or the layout of the page changes, the test is failing.

For instance:

If we want to make any usability changes to this text (or the text found in StandardTests()), the test fails. If we think tests need to pass, and it is as important as an API working, would we make an API this brittle?

What is this really testing? I think it's really trying to test this function:

update_get_available();

and perhaps update_get_projects()
and perhaps update_get_project_data().

If so, doesn't it make more sense to write units which test those pieces individually, than to test that the whole page comes up looking exactly like "XYZ is out of date " ? It would be less likely to fail, and when it did fail, we'd know exactly where it failed. It would also run faster.

I'm not saying we can decouple those functions from everything else in Drupal. Drupal just isn't built that way (yet). But we can at least test functions and not page loads.

From http://c2.com/cgi/wiki?UnitTest:

"Unit" casually refers to low-level test cases written in the same language as the production code, which directly access its objects and members.

Under the strict definition, for QA purposes, the failure of a UnitTest implicates only one unit. You know exactly where to search to find the bug.

Sadly, I don't have any time (paid or otherwise) to fix this, so it will remain an impediment to getting PM into core unless we turn these into unit tests, or alter them as part of the PM patch.

Best,
Jacob

The descriptions of the test groups themselves claim they're "functional tests" not "unit tests" for update status. Functional tests aren't as good for some things as unit tests, but they're better than no tests, which is exactly what we had up until now.

In terms of making update status easier to unit test, see comment #4. The APIs weren't originally written with unit-testability in mind, and haven't been refactored since. That's high on my list to try to accomplish before the hard freeze, since better tests for this module is an important goal.

And no, the existing tests aren't just testing update_get_available() and update_get_projects(). They're testing the whole stack, but in particular, the heart of the matter: update_calculate_project_data(). That's a giant function crying to be split up into smaller pieces, among other reasons so it can be more easily unit tested. That's exactly the kind of thing that I'm planning to do in #238950: Reduce RAM resource consumption and/or #361563: Update Notifications causes SQL Server to go away.

Core is full of functional tests, brittle tests, poorly written tests, down-right false tests. In the D7 rush to embrace a whole new development model and be able to say "see, we've got tests!", large swaths of tests were thrown at core with very little concern. Frankly, the developer community didn't have much experience with how to write reasonable tests, people didn't know how to review them, and the basic mantra was "if the bot passes, ship it!". Over the life of D7, that seems to have shifted a little bit, and some people are getting more savvy about writing and reviewing the tests, but it's still a long way to go. I think it's pretty unreasonable to expect an entire community to go from 0 to 100% in 1 release cycle...

Anyway, yes, now that I've been working on D7 core development again, I find myself running into the limitations and failings of our tests and test infrastructure. So, I'm trying to fix tests as I go (e.g. #582956: FormsTestCase::testRequiredFields() is broken in various ways). In fact, just days after I said this patch was RTBC, I had to re-write most of it for #591632: Refactor tests to allow testing contrib. ;) Anyway, like everyone else, we're expected to alter any existing tests (false, brittle or otherwise) that a patch we write are causing to fail. So, you get to have the same fun in the PM patch. ;)

The good news is that I'm trying to improve these tests, and I'd like to make as many as possible unit tests, API refactorings permitted... Sadly, I'm not getting paid to work on any of this either, and I have lots of other things I *should* be doing, but I'm trying to do my part to help core get better.

Automatically closed -- issue fixed for 2 weeks with no activity.