search

mikebronner / laravel-model-caching

Eloquent model-caching made easy.
MIT License
2.24k stars 214 forks source link

~ fixes all method not recognising change to global scope params #343

Closed Drewdan closed 4 years ago

Drewdan commented 4 years ago

Fixes #342

I have made these changes and it seems to fix the issues I raised in issue #342

The PR adds more detail to the key, uncluding the scope bindings - this means when we reach into the cache, we take into account the global scope bindings (if any) and use this to generate a more accurate key

mikebronner commented 4 years ago

@Drewdan Awesome that you got started on this PR!! Thanks a bunch :) Would you mind sticking with it just a bit more to get the Travis check to pass? Thanks!

Drewdan commented 4 years ago

Happy to help, I will take a look at the failing tests now :)

Drewdan commented 4 years ago

@mikebronner - I cannot seem to recreate the test failure I am getting in the pipeline on travis - it does not appear locally. Are you able to shed any light on it? I am a little stuck :(

mikebronner commented 4 years ago

Hi @Drewdan no worries about the Nova tests, they will only run on master, not on PRs.

Drewdan commented 4 years ago

My pipeline appears to be failing because of them, is that to be expected?

Drewdan commented 4 years ago

Hi @mikebronner -The travis pipeline is failing because of some Nova tests, that said from the look of it, they should not be running: image

I did try to modify the phpunit and travis yml files, but, I did not get much luck!

mikebronner commented 4 years ago

Yea, no worries .. the tests will pass once integrated.

dmason30 commented 4 years ago

@mikebronner The nova tests ran in the PR because you have removed the /** @group nova */ annotation from all the nova tests. The PR travis setup excludes the tests by using this group.

mikebronner commented 4 years ago

@dmason30 Thanks for setting me straight! LOL I didn't realize they were there for that. I will take a look at fixing that using the test suite instead of the test group.

dmason30 commented 4 years ago

@mikebronner I experimented with that but there is no way to --exclude-testsuite via a command argument. Solutions:

  1. add --testsuite Integration,Feature in the PR test runner
  2. add another phpunit.xml.pr and point to that in the PR test runner but this seems extreme.
  3. as before, @group nova and keep using the --exclude-group nova argument in the PR test runner.

I would recommend if making any changes to the tests around nova maybe do them via a PR or just create a dummy PR after to check its still skipping them as expected rather than straight to master.

mikebronner commented 4 years ago

@dmason30 I went ahead and implemented using test suites instead of groups, and got it working. But then went a step further and moved away from travis to GitHub Actions. No more issues, PRs should run fine now as well. 🤞