-
Notifications
You must be signed in to change notification settings - Fork 2
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
Tests for enabling and disabling modules and access to their content types #469
Conversation
…tent types. Requires localgovdrupal/localgov_geo#118 Notes: - Should switch to use the user and group roles we define in the submodules. - Why does Log Out not work? It does outside of tests. - Adding tests for global (non-group path) creation of content when it's included in the access per domain.
Sometimes the repeated / from domain and path matter. Sometimes they don't. Here they do.
New route option for redirecting when access is denied to a CSRF protected route https://www.drupal.org/node/3152693 Not sure what it was doing in 10.2 now to work. But this shouldn't break that as it checks for the form first.
…cess-tests-enable-disable-modules
Already exists in optional in localgov_microsites_group_term_ui and localgov_microsites_blogs. Depending on install order one of these can be already existing in configuration when the news config is installed.
Includes tests that will fail that are fixed by #470 |
…tests-enable-disable-modules
Need to test this locally, as 10.3 is not installing on our automated workflows. |
@stephen-cox What I noticed and was trying to fix with the commits there is that the project composer.json was requiring
However, if I put in
So I guess... well it would be nicest if it did the appending automatically but didn't version fail? But otherwise should the lines to add -dev go? I'm now not sure. But I think it is the version constraint on drupal core in the project composer.json that was causing the issue for not running the tests. |
@ekes @finnlewis I have fixed testing against Drupal 10.3 with: #482 Do we want to merge this PR? Otherwise you can merge the |
@stephen-cox this is the one we want to start with. |
I've fixed the coding standards errors and there's another issue for the deprecated code issues #483 The tests look correct to me and are failing because they pick up genuine issues. Are we just adding tests here or are we trying to fix stuff? There are a load of PRs just adding tests and they are now all mixed up as well, which is quite confusing. If we're just adding tests can we create one PR for them and close all the others? We could then detail all the issues found so we can start fixing them. |
Failing tests should be fixed in 470
|
Thanks @stephen-cox ! The intention was for those tests to fail here, so we can confirm #470 fixes them. I've merged your commits from here to #470 / https://github.com/localgovdrupal/localgov_microsites_group/tree/feature/464-access-disabled-modules--plus Let's close this one in favour of #470 We agree that the tests are testing the right thing, which did break, and hopefully will not break in the future. |
Requires localgovdrupal/localgov_geo#118
Will fix #464
To do:
Bonus: