Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Loader and Config Unit Test Improvements #1744

Merged
merged 18 commits into from

4 participants

@dchill42

These changes employ vfsStream to test loading libraries, helpers, config files, etc. without relying on existing nor generated files. This approach divorces the Loader from the objects it loads, focusing the tests on the loading process and preventing false negatives due to broken modules. The same mechanism supports testing the config file loader in Config.

The other key change is making Loader independent of the actual Config module, instead simply testing the load->config() feature against a mockup. Now Config can test the inner workings of its own file loader in its own unit test.

Another addition is a driver() test for the Loader module. This new test is dependent on some code changes included in #353. Until that request is merged, Loader_test::test_driver will fail.

Signed-off-by: dchill42 dchill42@gmail.com

@dchill42 dchill42 Improved VFS usage in Loader and Config units, added Loader driver te…
…st, and moved config load testing to Config unit

Signed-off-by: dchill42 <dchill42@gmail.com>
6030719
@dchill42

@philsturgeon Here's the Loader test improvements we discussed.

Ideally, autoloading should be tested too, but _ci_autoloader() will only look at the config file in APPPATH, which can't be redefined during the unit test. I think the right solution there is to actually set a Loader property to APPPATH and reference that in the method. That way, I can override the path in the mock Loader to reference the VFS instead, and apply a custom autoloader.php out of memory for the test.

This didn't seem like a good idea for the first draft of these changes. But, if you think that plan would fly, I'm happy to incorporate it in this request or add it as a separate request.

@philsturgeon

Looks good. When #353 is in we can merge this.

@dchill42

@philsturgeon Now that #353 has been pulled, we should be on deck with this. Do you think it's OK to change CI_Loader::_ci_autoloader() in this branch to use a member variable for referencing APPPATH? This would allow the path to be overridden in the unit test so autoloading can be tested with a config file in VFS.

I can add a commit in about 10 minutes with the proposed change if you agree it's worth looking at in this pull.

@dchill42

Meh - went ahead and did it anyway. I can revert that last commit if it's a dealbreaker.

@dchill42 dchill42 Better way - autoloader uses first config path
Signed-off-by: dchill42 <dchill42@gmail.com>
eeb6a48
@dchill42

@alexbilbie I guess maybe now you're the guy who can merge this, if you think it's a good set of changes.

@dchill42

@alexbilbie Merge me! Merge me! You like unit tests...

@narfbg
Owner

If the first element of _config_paths is always APPPATH - why do we need those changes to the Loader class?

@dchill42

@narfbg The reason was that when testing, it's much better for the first element to be vfs://application instead of APPPATH on the real filesystem. Using VFS gives finite control over the test-time content being loaded in terms of the ability to verify a successful load and also isolation from the other classes. Some broken library (or whatever) shouldn't give a false negative on the Loader, when it's doing its job correctly.

That having been said, I just looked over this request and realized that I've already incorporated all of it into #1818, which is looking somewhat promising for being merged. In that setup, I established an even better way to override the paths for unit testing in a top-down kind of way. In addition, #1818 consolidates all config file loading into a single CI_Config method, so that modified code gets relocated anyway. I'm thinking maybe I should just close this one and hope that HMVC gets picked up.

@narfbg
Owner

We've got feedback from EllisLab on #1818 and it looks promising, but it would need lots of testing and so it will have to wait for after the 3.0 release. Tests for the Loader and Config classes however do not need to wait.

With that being said, what's wrong with defining APPPATH to be vfs://application when in "test mode"? Again - tests are good, but actual code shouldn't be altered just so that unit testing gets better.

@ckdarby

I think this is an extremely poor decision to move this beyond 3.0 because adding HMVC is such a large change that it being apart of any 3.X series would make upgrading to 3.0 pointless until the HMVC 3.X came out.

This has been sitting long enough in the wings waiting for feedback that I feel like Ellislab dropped the ball on this & is now playing catch up but doesn't want to dedicate the time to catch up to where 3.0 should be & now instead are just cutting things to make 3.0 fit a reasonable schedule.

@narfbg
Owner

350+ lines of changelog entries, lots of (even though mostly small) new features, a major speed boost, change in the license, etc. You can't say that there's no reason to upgrade to 3.0. And with just a few tasks remaining to be done before the release - it may happen anytime soon. It could've been released 2 months ago and you still wouldn't have HMVC with it.

Truth is - this is a major new feature and a massive change of how things work down to the code, so it's not like we can merge it now and release tomorrow. It needs to be bullet-proof and I agree - it shouldn't be released yet.

@ckdarby

I just don't want to see this pushed off another year like it has been

@dchill42

@narfbg I agree with your principle about APPPATH 100%. I didn't change APPPATH because I didn't want to break anything that should actually be looking in the repo's application/ directory. We have a bunch of core classes, libraries, and helpers that may be depending on finding real files in the actual directory during testing.

I can sort all of those out and potentially establish a system of unit testing purely against a VFS APPPATH, but I was not sure this request was the right place to do so. If you're happy to have that change implemented here, and will merge this request with the much broader range of unit test files it will touch, then I'm happy to improve the unit test environment that way. It's your call.

I'm also open to other suggestions how to handle this.

@narfbg
Owner

@dchill42 How I see it is that this PR is about adding/improving unit test, so yes - feel free to add anything tests related. It might be a better idea to do that in chunks however.

On APPPATH - when you use CodeIgniter, you're defining that in index.php. How hard can it be to define it differently for the tests?

@dchill42

For unit testing, APPPATH is defined in tests/Bootstrap.php for the entire unit test run. The trick is identifying where in the existing tests that path is being used and mocking up suitable replacement files in VFS for those scenarios. I'll also have to rearrange where we setup VFS, as right now that happens after Bootstrap.php runs. Fear not, though - I'm sure I can get that all straightened out, and I firmly believe the unit test framework will be better off for it.

I will ask for a clarification of "chunks" though. With regard to the whole PR, all the related changes will have to be incorporated in order for the unit tests to pass, of course. If you just mean sensibly grouped commits, that's fine - I can do that. If you mean something else, then I need a clue. :smile:

@narfbg
Owner

I meant grouping of change sets. :)

dchill42 added some commits
@dchill42 dchill42 Merge branch 'develop' of github.com:/EllisLab/CodeIgniter into load_…
…config_units
2716398
@dchill42 dchill42 Integrated vfsStream better and made paths constants VFS-based
Signed-off-by: dchill42 <dchill42@gmail.com>
7ecc5cd
@dchill42 dchill42 Merge branch 'develop' of github.com:/EllisLab/CodeIgniter into load_…
…config_units
93ec20b
@dchill42 dchill42 Adapted DB for VFS changes and fixed Common case in Bootstrap.php
Signed-off-by: dchill42 <dchill42@gmail.com>
e9435dc
@dchill42 dchill42 Reverted autoloader change now that APPPATH is in VFS
Signed-off-by: dchill42 <dchill42@gmail.com>
92e5511
@dchill42 dchill42 Adapted Config load test to VFS APPPATH
Signed-off-by: dchill42 <dchill42@gmail.com>
e3621cc
@dchill42 dchill42 Merge branch 'develop' of github.com:/EllisLab/CodeIgniter into load_…
…config_units
63391f7
@dchill42 dchill42 Merge branch 'develop' of git://github.com/EllisLab/CodeIgniter into …
…load_config_units
9e4ebf1
@dchill42 dchill42 Raised CI_Config test coverage to 100%
Signed-off-by: dchill42 <dchill42@gmail.com>
3a43f13
@dchill42 dchill42 Merge branch 'load_config_units' of github.com:dchill42/CodeIgniter i…
…nto load_config_units
b1aad96
@dchill42 dchill42 Raised CI_Loader test coverage to 93%
Signed-off-by: dchill42 <dchill42@gmail.com>
4f42be5
@dchill42 dchill42 Added documentation of new unit test tools and VFS additions
Signed-off-by: dchill42 <dchill42@gmail.com>
cf99aac
@dchill42 dchill42 Merge branch 'develop' of git://github.com/EllisLab/CodeIgniter into …
…load_config_units
c2f59ef
@dchill42 dchill42 Replaced evals with getMock usage in Loader tests
Signed-off-by: dchill42 <dchill42@gmail.com>
8889db7
@dchill42

I think this is good now, but with one exception - I'd like to see the database Loader calls covered. I think the only way to pull that off while avoiding conflicts over the global DB() function is to use namespaces. Does anybody have any input on applying this 5.3+ PHP feature to a unit test?

Bear in mind, this will not effect the usability of the class in 5.2.4, it will only put 5.3 code in the unit test. I can also add a PHP version check to the tests in question and skip them if the test environment doesn't support it.

Travis only supports running the tests on 5.3+ anyway, so there will not be any conflict there. In fact, some of the PHPUnit code uses namespaces (in MockObject), which may be one of the reasons Travis does not offer 5.2 testing.

@narfbg
Owner

We can't run 5.2 on Travis and whoever runs tests locally would most likely even have 5.4, so go for it. We don't need to support 5.2 on the tests.
Just do it in a separate pull request - we should get this one in now.

@narfbg narfbg merged commit bf93f93 into bcit-ci:develop

1 check passed

Details default The Travis build passed
@narfbg narfbg referenced this pull request
Closed

HMVC with Core Unit Tests #1818

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Aug 28, 2012
  1. @dchill42

    Improved VFS usage in Loader and Config units, added Loader driver te…

    dchill42 authored
    …st, and moved config load testing to Config unit
    
    Signed-off-by: dchill42 <dchill42@gmail.com>
Commits on Aug 29, 2012
  1. @dchill42
  2. @dchill42

    Added autoloader unit test with minor supporting change in Loader

    dchill42 authored
    Signed-off-by: dchill42 <dchill42@gmail.com>
Commits on Aug 30, 2012
  1. @dchill42

    Better way - autoloader uses first config path

    dchill42 authored
    Signed-off-by: dchill42 <dchill42@gmail.com>
Commits on Oct 10, 2012
  1. @dchill42
Commits on Oct 12, 2012
  1. @dchill42

    Integrated vfsStream better and made paths constants VFS-based

    dchill42 authored
    Signed-off-by: dchill42 <dchill42@gmail.com>
  2. @dchill42
Commits on Oct 14, 2012
  1. @dchill42

    Adapted DB for VFS changes and fixed Common case in Bootstrap.php

    dchill42 authored
    Signed-off-by: dchill42 <dchill42@gmail.com>
  2. @dchill42

    Reverted autoloader change now that APPPATH is in VFS

    dchill42 authored
    Signed-off-by: dchill42 <dchill42@gmail.com>
  3. @dchill42

    Adapted Config load test to VFS APPPATH

    dchill42 authored
    Signed-off-by: dchill42 <dchill42@gmail.com>
Commits on Oct 15, 2012
  1. @dchill42
Commits on Oct 20, 2012
  1. @dchill42
Commits on Oct 21, 2012
  1. @dchill42

    Raised CI_Config test coverage to 100%

    dchill42 authored
    Signed-off-by: dchill42 <dchill42@gmail.com>
  2. @dchill42
Commits on Oct 22, 2012
  1. @dchill42

    Raised CI_Loader test coverage to 93%

    dchill42 authored
    Signed-off-by: dchill42 <dchill42@gmail.com>
  2. @dchill42

    Added documentation of new unit test tools and VFS additions

    dchill42 authored
    Signed-off-by: dchill42 <dchill42@gmail.com>
Commits on Oct 23, 2012
  1. @dchill42
  2. @dchill42

    Replaced evals with getMock usage in Loader tests

    dchill42 authored
    Signed-off-by: dchill42 <dchill42@gmail.com>
Something went wrong with that request. Please try again.