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

[5.8] Bind mix helper to the container to allow swapping in tests #26289

Merged
merged 1 commit into from Oct 29, 2018

Conversation

Projects
None yet
3 participants
@timacdonald
Contributor

timacdonald commented Oct 29, 2018

This PR binds the mix helper functionality to the container to allow it to be swapped easily during testing.

Why is this even needed?

If a view is using the mix helper to retrieve an asset, and the key passed to the mix helper is created dynamically i.e. related to a model attribute, you need to:

  1. Create the manifest in the public directory, or wherever you have specified it goes in the webpack.mix.js
  2. Add the file as a key value pair in the manifest.
  3. Cleanup the file after the test has run.

Example

You have a Business class. The class has a resource_key that relates to assets, such as a background image. This key is generated, in your tests, using Faker.

{{ mix("backgrounds/{$business->resource_key}.png") }}

In order for me to be able to test this view I would need to do something like this...

$business = factory(Business::class)->create();

touch(public_path('mix-manifest.json'));

file_put_contents(public_path('mix-manifest.json'), json_encode([
    "/backgrounds/{$business->resource_key}.png" => "/whatever.png",
]));

// run a feature test that hits a view with the  mix helper as shown above...

unlink(public_path('mix-manifest.json'));

With it bound to the container we can essentially "fake" the mix method:

$business = factory(Business::class)->create();

$this->app->swap('mix', function () { });

// run a feature test that hits a view with the mix helper as shown above...

Obviously the assets that go through the mix helper are known when writing your test, so you could create a list of known resource keys that you know are going to be available, however this means you have to have run npm run dev to ensure the mix manifest is available, and if the known assets change you need to update either your tests or your model factories.


To keep the PR focused on what is actually happening I have not made any changes to the helpers code, but moving it to a dedicated class will mean that, if you wanted, we could also:

  • Remove the static variable and make it a class property (because it is registered as a singleton).
  • Make a bunch of class helper methods to make what is happening a bit clearer in the conditionals / code blocks.
  • Remove the dependence on global methods.

@timacdonald timacdonald changed the title from bind mix helper to the container to [5.8] Bind mix helper to the container to allow swapping in tests Oct 29, 2018

@timacdonald timacdonald force-pushed the timacdonald:bind_mix_helper branch from b2f00d3 to c979a30 Oct 29, 2018

@timacdonald timacdonald force-pushed the timacdonald:bind_mix_helper branch from c979a30 to c123a7b Oct 29, 2018

@taylorotwell taylorotwell merged commit c123a7b into laravel:master Oct 29, 2018

2 checks passed

continuous-integration/styleci/pr The analysis has passed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@austenc

This comment has been minimized.

Contributor

austenc commented Nov 30, 2018

Apologies for commenting on the other PR earlier today, meant to comment here. Is there a possibility of this being merged for 5.7? Doesn't seem like it would change things too much.

@austenc

This comment has been minimized.

Contributor

austenc commented Dec 2, 2018

Did a quick test on framework version 5.7.13 and our tests pass without having to spoof the contents of the mix-manifest.json file (just touching it is enough). I see from the logs that the Unable to locate Mix file errors are being logged. Prior to these changes, the exception never got thrown, it would just report. While I agree that throwing the exception is an improvement to framework code, your other PR should be merged so that we can more easily account for the behavior change this PR introduced.

Before, in the CreatesApplication trait before, we had this:

touch(public_path('mix-manifest.json'))`

Now, we have to do something like this:

        touch(public_path('mix-manifest.json'));
        touch(public_path('app.js'));
        touch(public_path('style.css'));
        touch(public_path('print.css'));
        file_put_contents(public_path('mix-manifest.json'), json_encode([
            "/js/app.js" => "/app.js",
            "/css/style.css" => "/style.css",
            "/css/print.css" => "/print.css",
        ]));

Looking forward to being able to toggle the mix stuff in the container!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment