Phpunit tests fail on master branch

[chihab]

• I'm submitting a ...
• bug report
• feature request
• support request
• other (Please specify)

Before submitting a PR for #356, there is an issue to be fixed, otherwise the PR for #356 would make the build failing on Travis.

If we follow these steps locally, the build fails on phpunit and returns this error:

git clone https://github.com/jadjoubran/laravel5-angular-material-starter.git
cd laravel5-angular-material-starter
composer install
// set database env in .env
php artisan clear-compiled
php artisan optimize
php artisan key:generate
php artisan jwt:generate
npm install -g gulp bower && npm install && bower install
php artisan migrate --force && gulp
vendor/bin/phpunit

Phpunit will comlain:

Warning: Uncaught exception 'ErrorException' with message 'require(../angular/critical.css): failed to open stream: No such file or directory' in /private/tmp/laravel5-angular-material-starter-compposer/storage/framework/views/299651b84c1d7e1a724e97a14846d9c6a4d554e7.php:17

On Travis, the build passes because the "../angular/critical.css" refers to the Travis cloned repo and not the composer create-project repo inside it ! So, the file is found in the upper folder and so the build passes ! These two directory levels on Travis build will be removed after #356 fix.

The fix is to be made on that line I suppose (index.blade.php: 17)
<style><?php require("../angular/critical.css") ?></style>

Important: even if phpunit fails, the applications runs well (with php artisan serve).

I'll submit the PR on #356 once all phpunit tests run successfully.

Question:
How do you tell Packagist that the current stable version is tagged 3.2.1 ? (and not the one on master branch)

• Laravel & Angular version: master branch

[jadjoubran]

Are you sure it's failing on master? It's all green here
Packagist considers the latest tag to be stable (unless it had a beta or alpha in it). It never considers master as stable

[chihab]

@jadjoubran, it is all green for the reason I've explained:

On Travis, the build passes because the "../angular/critical.css" refers to the Travis cloned repo and not the composer create-project repo inside it ! So, the file is found in the upper folder and so the build passes ! These two directory levels on Travis build will be removed after #356 fix.

We have something like this:

• laravel5-angular-material-starter (git clone)
  • angular
    • critical.css
  • server.php
  • ...
  • laravel5-angular-material-starter (composer create-project, phpunit is run here !)
    • angular
      • critical.css
    • server.php

require("../angular/critical.css") inside laravel5-angular-material-starter/laravel5-angular-material-starter finds critical.css inside laravel5-angular-material-starter/angular/critical.css and not laravel5-angular-material-starter/laravel5-angular-material-starter/angular/critical.css as it "should".

So the test passes !

Try to run phpunit on your locally cloned repo, it will fail.

The require("../angular/critical.css") has to be fixed.

I hope it is clear now

[hackur]

I probably overcomplicated things on my application.. but I created a whole new task for my critical .scss file and compile it first into public/css... then I require it inside of my index file using:

<!-- Critical CSS (Applied Immediately) -->
<style type="text/css">
<?php require( public_path( "css/critical.css" ) ) ?>
</style>

This way I can use sass in my critical CSS... and I don't have a random *.css file sitting inside of my angular directory all by itself.

[jadjoubran]

@chihab okay it makes sense now, thanks!
@hackur what you're mentioning is actually quite interesting..
being able to use sass is useful mainly because they give us access to the variables.. (mainly for theming)
and it seems this also solves the issue mentioned by @chihab
Would you be able to send a PR for that? Or just open a new issue and describe the process so I can replicate it

[chihab]

@jadjoubran, could you please explain what is the purpose of that critical.css and its require ? I see that it includes some app-shell style definitions, probably related to pwa then..

[jadjoubran]

@chihab yes indeed, this is concerned with the app shell
It is inlined so that it doesn't block critical path rendering
That's also why the main script is loaded with async and the main styles are dynamically injected
this allows to display the app shell in under than 1s (under 300ms on fast connections)

[chihab]

@jadjoubran why not simply inline app-shell css in the html ? Without an external critical css file and a need to require it in some way.

    <style>
    body{
        margin: 0;
    }
    #app-shell-header{
        background-color: white;
        height: 64px;
    }
    #app-shell-header img{
        padding: 13px 0;
        margin-left: 5%;
    }
    #app-shell-content{
        background-color: #00A4C6;
        min-height: 600px;
    }
    </style>

If there is really a need to make it external, for design purpose, maybe using a gulp css inlining task like https://www.npmjs.com/package/gulp-inline-css would solve the problem I guess.

[jadjoubran]

@chihab yeah it used to be simply inlined in the HTML, but I thought about moving it to a different file for the developer experience.. especially when the shell code goes slightly larger than this
gulp-inline-css wouldn't help because that's mainly useful for emails.. we don't need to change the "style" attribute of elements

btw wouldn't @hackur's suggestion work? using public_path() helper

and then I liked his idea because converting this critical.css file into scss will allow us to use the same variables that we have (mainly the color variables)

[chihab]

Okay @jadjoubran it makes sense. :)

Following @hackur 's solution, we would have this then on gulpfile (along with the public_path on require)

        .sass('./angular/**/*.scss', 'public/css')
        .sass('./angular/critical.scss', 'public/css/critical.css')

The critical.css would be renamed to critical.scss.

If we'd like to move critical.scss to material folder we would have to the some wildcard playings to exclude it from the first .sass call.

If that solutions suits you, I'll make a PR on that issue.

Really want to send a PR for Travis issue #356 , the current CI builds are confusing. :)

[jadjoubran]

Yes looks great 😄 except for one issue, it seems that crucial.scss will be processed with the regular sass task.. which is not what we want
critical css should be only inlined, and not included in the final.css
So even without moving the critical to the material folder, we need some way to exclude it.. !angular/critical.scss might work

Thanks for your contribution 😄 Your PR is more than welcome

[chihab]

Oh yes, I see, I'll try to send the two PRs today. :)

[jadjoubran]

re-opened to fix issues discussed in #356