Skip to content
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

Fix todos in admin view functional test #370

Merged
merged 6 commits into from
Sep 3, 2024

Conversation

alansaun92
Copy link
Contributor

I noticed a couple of todos mentioned in the functional test AdminViewTest.

So I have moved the creation of the admin user into the setup function and added in a kernel test to confirm that the admin url that we are expecting is the one set on the manage alert banners view.

Copy link
Member

@ekes ekes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does the AdminViewUrlTest::testAdminViewUrl() need to be a whole new test, rather than another method on AdminViewTest?

@andybroomfield
Copy link
Contributor

@ekes, though the new admin view url is a kernel test so should be quicker (the AdminViewTest is a functional test). If we wanted to keep it there we could add the code to the method?

Copy link
Contributor

@andybroomfield andybroomfield left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm happy with this. We could move this back into the functional AdminViewTest but technically the admin alert banner view is optional, as there is a default alert banner entity collection provided, so this is testing that the alert banner view is set up correctly to the desired URL (overriding the entity collection).

@ekes
Copy link
Member

ekes commented Sep 3, 2024

@ekes, though the new admin view url is a kernel test so should be quicker (the AdminViewTest is a functional test). If we wanted to keep it there we could add the code to the method?

I'd overlooked it was a Kernel test.

All good.

@ekes ekes merged commit 8c4fafd into 1.8.x Sep 3, 2024
8 checks passed
@ekes ekes deleted the fix/todos-in-admin-view-functional-test branch September 3, 2024 11:22
@andybroomfield andybroomfield mentioned this pull request Sep 16, 2024
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants