mblarsen commented 3 years ago

The problem with the routes not being set is that the ApiRouteServiceProvider is overriding register(). Remove it and you got your routes back.

-    /**
-     * register services.
-     *
-     * @return void
-     */
-    public function register()
-    {
-        //
-    }

The RouteServiceProvider calls the map() function indirectly from the register() function by setting up a 'booting' hook. But when you override the register call this never happens.

cbj4074 commented 3 years ago

@mblarsen Indeed you are correct! Removing that register() method does re-enable all the routes.

There looks to be a related issue that still causes all the methods in ApiRouteTest to fail. It doesn't seem like the map() method in MockApiRouteServiceProvider is ever called.

I can do a bit more digging shortly, if nothing jumps-out at you!

mblarsen commented 3 years ago

Oh I thought that was related to the order changes. If not I'll look again.

cbj4074 commented 3 years ago

There was another Order-related issue that was causing test-failures, but I just pushed a commit that fixes that.

So, with your earlier suggestion (removing the register() override), and the temporary block-comment around the Order-related route definitions, the only failing tests are those in ApiRouteTest.

I'm sure you can figure it out much more quickly than I could! :)

mblarsen commented 3 years ago

@cbj4074 the issue was that the mock service provider wasn't registered in the app container.

ps: I don't get the feedback styleci provides at all.

codecov[bot] commented 3 years ago

Codecov Report

Merging #191 into master will decrease coverage by 0.84%. The diff coverage is n/a.

@@             Coverage Diff              @@
##             master     #191      +/-   ##
============================================
- Coverage     71.79%   70.95%   -0.85%     
- Complexity      405      409       +4     
============================================
  Files           109      113       +4     
  Lines          1060     1105      +45     
============================================
+ Hits            761      784      +23     
- Misses          299      321      +22

Impacted Files	Coverage Δ	Complexity Δ
app/Providers/ApiRouteServiceProvider.php	`100.00% <ø> (ø)`	`3.00 <0.00> (ø)`
app/Observers/LabelObserver.php	`57.14% <0.00%> (-42.86%)`	`6.00% <0.00%> (+5.00%)`	:arrow_down:
app/Observers/GenreObserver.php	`71.42% <0.00%> (-28.58%)`	`6.00% <0.00%> (+5.00%)`	:arrow_down:
app/Console/Kernel.php	`75.00% <0.00%> (-25.00%)`	`2.00% <0.00%> (+1.00%)`	:arrow_down:
app/Observers/UserObserver.php	`75.00% <0.00%> (-25.00%)`	`6.00% <0.00%> (+5.00%)`	:arrow_down:
app/Observers/AlbumObserver.php	`84.61% <0.00%> (-15.39%)`	`6.00% <0.00%> (+5.00%)`	:arrow_down:
app/Observers/ArtistObserver.php	`63.63% <0.00%> (-11.37%)`	`6.00% <0.00%> (+5.00%)`	:arrow_down:
app/Artist.php	`100.00% <0.00%> (ø)`	`8.00% <0.00%> (ø%)`
app/Repositories/CrudRepository.php	`100.00% <0.00%> (ø)`	`3.00% <0.00%> (ø%)`
app/Repositories/SongRepository.php	`100.00% <0.00%> (ø)`	`4.00% <0.00%> (ø%)`
... and 8 more

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update c7987aa...54922ec. Read the comment docs.

cbj4074 commented 3 years ago

@mblarsen Oh, good call! That's funny... how did it ever work indeed?!

It looks like we're back to all-green, so let's go ahead and merge this in, if it looks good to you, too.

Regarding Style-CI, it looks like it's mostly whitespace on empty lines, trailing commas, etc. I'm content just to let it commit directly to PRs to fix the issues it finds. We may find that we want to tweak the rules, though.

indiehd / web-api

Laravel 8 Upgrade #191

Codecov Report