After the DrupalOrg live endpoint and plugin was made live, the mock should no longer be offered as an option and we shouldn't need to worry anymore about regenerating fixtures, etc.
It might still be useful to have a DB driven plugin in the tests folder tho, so we can still keep it as many tests use it.
Remove mock related code to update fixtures and demote the plugin to the test folder
| Comment | File | Size | Author |
|---|---|---|---|
| #9 | mock-source.png | 21.7 KB | prashant.c |
Show commandsStart within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
changes
Comments
Comment #3
lostcarpark commentedKeeping the mock API available for tests sounds useful.
Comment #4
fjgarlin commentedI've done a big refactoring + cleanup and most of the tests are passing as well as all the linting steps.
There are some tests still not passing, but most of them are green already. If somebody would be willing to help here that'd be great as my available time is limited. I can continue working on it, but it might be a few days until I get to it again.
Currently only two test files are failing:
* The failing Kernel test seems to fail because the “inserts” that run on the install hook either don’t run or don’t find the fixture files.
* The failing Functional test is failing to find the Browse tab.
Comment #5
fjgarlin commentedGreen tests all over!! I still want to do more clean up but this is a great milestone in this initial refactoring.
Comment #6
fjgarlin commentedFollowing up on #5 (all tests green). I have cleaned up any code that was related to the initial drupalorg_mockapi and I have moved only the parts that were needed to the "tests" folder and renamed the plugin slightly.
I've tested via Drupalpod and things look good too. This is now ready for review. It's a big MR but it's a big cleanup, so it's expected.
Comment #7
fjgarlin commentedRebased from latest and trying to fix the nightwatch test.
Comment #8
fjgarlin commentedAll green after rebase. Back to needs review.
Comment #9
prashant.cI tried to test it when this Mock source was enabled. It does not get completely removed, Only the source name got removed.
Comment #10
prashant.cNeed a hook_update to delete the config value
drupalorg_mockapifrom theproject_browser.admin_settings.Comment #11
prashant.cComment #12
fjgarlin commentedYeah, this was a known issue that was discussed in the MR. Thanks for the commits @prashantc.
We also need to remove the tables. I named the hook "project_browser_update_9019" as suggested by Chris.
I added the above and also sorted out the missing hooks using "hook_update_last_removed".
Ready for review again.
Comment #13
lostcarpark commentedIf someone was a couple of versions behind and installs this, the runs "drush updb", they get the following:
Should we retain a dummy version of this class for legacy updates? I'd guess it's not something we'd want to keep when we move to core, so I'd guess we need to evaluate whether it's worth keeping in play in the short time.
Comment #14
fjgarlin commentedWhere in the code does that class appear so it gets executed somehow by "drush updb"? I definitely wouldn't want to keep a dummy version of the file. We just need to figure out why is this triggered if there are no instances of it in the code anymore.
Comment #15
lostcarpark commentedHmmm, I was assuming it was coming from one of the
_project_browser_populate_from_fixture()updates, but now I see they've all been removed, so I don't know what's causing this.Comment #16
fjgarlin commentedI’ll check this. Probably more cleanup needed somewhere.
Comment #17
fjgarlin commented@lostcarpark - there was still a file that needed to be removed. Done here: https://git.drupalcode.org/project/project_browser/-/merge_requests/554/...
Hopefully you won't get that warning anymore.
Comment #18
prashant.cError mentioned in #13 was getting triggered from the
drush.services.ymlfile.The
UpdateFixtureCommandsclass was removed therefore removed the drush file as well.Comment #19
fjgarlin commentedOn comment #16 I wrote:
And then we've done pretty much the same commit simultaneously.
@prashantc - I know that you're trying to help but it'd be great if we can coordinate on who's doing what. I'm happy if you want to take over but I just want to avoid duplicate efforts.
Comment #20
lostcarpark commentedAha! That seems to have done the trick. Update ran cleanly for me this time.
Comment #21
prashant.c@fjgarlin
Apologies and I agree to avoid duplicate efforts :), I encountered this error on cache clear as well, and then I found that the drush file is not required, unfortunately, we pushed the same change at the same time.
Comment #22
fjgarlin commentedIt's fine. I'm on slack as well if you need real time replies or checks.
Hopefully 🤞, this was the last change needed on this MR, but yeah, let's just make a quick comment or give a quick ping if something else comes up. Thanks for helping!
Comment #23
lostcarpark commentedI've carried out manual testing including upgrading the module, uninstalling, and installing. Everything seems to be working as expected.
I also reviewed the code changes and they look good.
All tests have passed in CI.
I think this is good to go RTBC.
Comment #26
chrisfromredfinWoooo! big things are happening. :)