-
Notifications
You must be signed in to change notification settings - Fork 4
Bundles UI tests CMS-333 #273
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
base: main
Are you sure you want to change the base?
Conversation
functional_tests/steps/bundles.py
Outdated
| mark_page_as_ready_to_publish(release_calendar_page,user) | ||
| release_calendar_page.save_revision().publish() | ||
| context.release_calendar_page = release_calendar_page | ||
| print(context.release_calendar_page.pk) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Leftover print statement
functional_tests/steps/bundles.py
Outdated
| Title = "Bundles UI Testing" | ||
| Summary = f"{Title} Summary" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should never capitalise the first letter of a variable name in Python. This is reserved for class names, and can create a lot of confusion.
Please read through this to understand Python naming conventions: https://peps.python.org/pep-0008/#prescriptive-naming-conventions
functional_tests/steps/bundles.py
Outdated
| context.page.get_by_role("button", name="Publish").click() | ||
| context.article_path.append(Title) | ||
|
|
||
| def get_url_for_article_edition(context): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You don't seem to be using this anymore?
functional_tests/steps/bundles.py
Outdated
| context.page.get_by_role("button", name="Save draft").click() | ||
| context.page.get_by_role("button", name="More actions").click() | ||
| context.page.get_by_role("button", name="Publish").click() | ||
| context.article_path.append(Title) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is article_path used for?
functional_tests/steps/bundles.py
Outdated
| context.page.get_by_label("Page explorer").get_by_role("link", name="Bundles UI Testing 2", exact=True).click() | ||
| context.page.get_by_role("row", name="Select Articles More options").get_by_label("Select").check() | ||
| context.page.get_by_role("link", name="Add a child page to 'Articles'").click() | ||
| Title ="Bundles UI Article Series Page" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto here, wrong naming convention
| # | 1 | Publishing Admin | Publishing Officer | {"role": "Publishing Admin", "creator_role": "Publishing Officer", "status": "In_Review", "preview_teams": true, "add_rel_cal": true, "add_stat_page": true} | | ||
| # | 1 | Publishing Admin | Publishing Admin | {"role": "Publishing Admin", "creator_role": "Publishing Admin", "status": "In_Review", "preview_teams": true, "add_rel_cal": true, "add_stat_page": true} | | ||
| # | 1 | Publishing Officer | Publishing Officer | {"role": "Publishing Officer", "creator_role": "Publishing Officer", "status": "In_Review", "preview_teams": true, "add_rel_cal": true, "add_stat_page": true} | | ||
| # | 1 | Publishing Officer | Publishing Admin | {"role": "Publishing Officer", "creator_role": "Publishing Admin", "status": "In_Review", "preview_teams": true, "add_rel_cal": true, "add_stat_page": true} | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If there is a reason why you have this commented out, it would be prudent to either leave a comment explaining why, or update the PR description so that this is clear.
Same on line 62, 63 and 211.
functional_tests/steps/bundles.py
Outdated
| if bool(bundle_dets["add_stat_page"]) and hasattr(context, "statistical_article_page"): | ||
| page = context.statistical_article_page | ||
| BundlePage.objects.create(parent=bundle, page=page) | ||
| # context.bundlepages.append(BundlePageFactory(parent=bundle, page=page)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Commented out code left here.
functional_tests/steps/bundles.py
Outdated
|
|
||
|
|
||
| @then("the {role} can preview the Release Calendar page") | ||
| def preview_release_calendar_page(context: Context, role: str) -> None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to highlight a thing picked up by the linter: you have an unused argument here (role). We've discussed this already that you can word your step differently to avoid the need of mentioning the role - this will be implied based on previous steps.
functional_tests/features/bundle_ui.feature:63 A User cannot save a bundle due to duplicate schedule -- @1.1 bundles functional_tests/features/bundle_ui.feature:64 A User cannot save a bundle due to duplicate schedule -- @1.2 bundles functional_tests/features/bundle_ui.feature:172 A user can approve a bundle -- @1.1 bundles functional_tests/features/bundle_ui.feature:173 A user can approve a bundle -- @1.2 bundles functional_tests/features/bundle_ui.feature:174 A user can approve a bundle -- @1.3 bundles functional_tests/features/bundle_ui.feature:175 A user can approve a bundle -- @1.4 bundles functional_tests/features/bundle_ui.feature:208 A user cannot approve a bundle due to lack of pages or datasets -- @1.1 bundles
MaciekBaron
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've noticed that in your latest commit you included a list of failing tests. They are failing because there are mistakes in your code. I have added a few comments to help you fix one of them ("A user can approve a bundle"). It's possible that these changes will also fix your other tests. They may also get resolved once you fix the things I mentioned in my other comments.
| mark_page_as_ready_to_publish(release_calendar_page, user) | ||
| release_calendar_page.save_revision().publish() | ||
| context.release_calendar_page = release_calendar_page |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Your test is failing because you are creating a new revision and publishing it after you have marked it as ready to publish. This resets it state.
There's no reason for you to do that anyway. Just remove that line to fix this.
| mark_page_as_ready_to_publish(release_calendar_page, user) | |
| release_calendar_page.save_revision().publish() | |
| context.release_calendar_page = release_calendar_page | |
| mark_page_as_ready_to_publish(release_calendar_page, user) | |
| context.release_calendar_page = release_calendar_page |
functional_tests/steps/bundles.py
Outdated
| bundle.release_calendar_page = context.release_calendar_page | ||
| BundlePage.objects.create(parent=bundle, page=page) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Next, your test will fail because you are assigning a release calendar page to the bundle and then adding it to the bundle as a bundled page. This is not allowed, and the validation will pick this up.
functional_tests/steps/bundles.py
Outdated
| @then("the logged in user can approve a bundle") | ||
| def can_approve_bundle(context: Context) -> None: | ||
| bundle_edit = f"Edit '{context.bundles[-1].name}'" | ||
| bundle_status = f"Bundle '{context.bundles[-1].name}' Approval' updated. Edit" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't correct and fails the test - the Approval' bit is unnecessary
| Scenario Outline: A User can edit a bundle | ||
| Given there is a <role> user | ||
| And there is a <creator_role> user | ||
| And there is a Statistical Analysis page approved by <creator_role> | ||
| And there is a Release Calendar page approved by <creator_role> | ||
| And there is a preview team | ||
| And the <role> is a member of the preview team | ||
| And there are <number_of_bundles> bundles with <bundle_details> | ||
| When the <role> logs in | ||
| Then the logged in user can find the bundle | ||
| And the logged in user goes to edit bundle | ||
| And the logged in user can add a release schedule | ||
| And the logged in user can add pages | ||
| And the logged in user can add preview team | ||
| And the logged in user can send bundle to moderation | ||
|
|
||
|
|
||
| Examples: bundles | ||
| | number_of_bundles | role | creator_role | bundle_details | | ||
| | 2 | Publishing Admin | Publishing Admin | {"role": "Publishing Admin", "creator_role": "Publishing Admin", "status": "Draft", "preview_teams": false, "add_rel_cal": false, "add_stat_page": false} | | ||
| | 1 | Publishing Admin | Publishing Officer | {"role": "Publishing Admin", "creator_role": "Publishing Officer", "status": "Draft", "preview_teams": false, "add_rel_cal": false, "add_stat_page": false} | | ||
| | 11 | Publishing Officer | Publishing Admin | {"role": "Publishing Officer", "creator_role": "Publishing Admin", "status": "Draft", "preview_teams": false, "add_rel_cal": false, "add_stat_page": false} | | ||
| | 1 | Publishing Officer | Publishing Officer | {"role": "Publishing Officer", "creator_role": "Publishing Officer", "status": "Draft", "preview_teams": false, "add_rel_cal": false, "add_stat_page": false} | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You have this test twice (line 69).
|
|
||
|
|
||
| #---- Bundle Cannot preview UI Tests ----- | ||
| Scenario Outline: A User cannot preview a bundle due to not member of associated preview-team |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| Scenario Outline: A User cannot preview a bundle due to not member of associated preview-team | |
| Scenario Outline: A User cannot preview a bundle due to not being a member of an associated preview-team |
| And there are <number_of_bundles> bundles with <bundle_details> | ||
| When the <role> logs in | ||
| Then the logged in user can approve a bundle | ||
| And the bundle pages are not live |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pages not being live isn't part of the scenario outline. Create a separate scenario if this is what you want to test - having said that, the ticket doesn't list this as a requirement.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah it didn't make sense has been remove
resolved 6c68056ac
| Scenario Outline: A user cannot approve a bundle due to authorisation | ||
| Given there is a <role> user | ||
| And there is a <creator_role> user | ||
| And there is a Statistical Analysis page approved by <creator_role> | ||
| And there is a Release Calendar page approved by <creator_role> | ||
| And there is a preview team | ||
| And the <role> is a member of the preview team | ||
| And there are <number_of_bundles> bundles with <bundle_details> | ||
| When the <role> logs in | ||
| Then the logged in user cannot approve a bundle | ||
|
|
||
| Examples: bundles | ||
| | number_of_bundles | role | creator_role | bundle_details | | ||
| | 1 | Viewer | Publishing Admin | {"role": "Viewer", "creator_role": "Publishing Admin", "status": "In_Review", "preview_teams": false, "add_rel_cal": true, "add_stat_page": true}| | ||
| | 1 | Publishing Officer | Publishing Officer | {"role": "Publishing Officer", "creator_role": "Publishing Officer", "status": "In_Review", "preview_teams": true, "add_rel_cal": true, "add_stat_page": true} | | ||
| | 1 | Publishing Admin | Publishing Admin | {"role": "Publishing Admin", "creator_role": "Publishing Admin", "status": "In_Review", "preview_teams": true, "add_rel_cal": true, "add_stat_page": true} | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This scenario is not very clear.
You should have separate tests: one for a viewer not being to approve bundles, and then another one for creator of the bundle not being able to approve their own bundle. This makes it clearer and more maintainable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This Test has now been removed as is no longer applicable
resolved 6c68056ac
| Scenario Outline: A User cannot create a bundle due to authorisation | ||
| Given there is a <role> user | ||
| When the <role> logs in | ||
| Then the logged in user cannot create a bundle | ||
| Examples: bundles | ||
| | role | | ||
| | Viewer | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's pointless to use a scenario outline if you only have one example. Convert this to a regular scenario to improve readability. Here and on lines 141 and 197.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand your comment but these are the exceptions to the success scenarios above
my logic/readability is that the exception should replicate the success scenario to the point of failure....
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry Ann, I don't understand your reply.
Scenario Outlines are used for parameterised scenarios, where the scenario is executed for each row in the examples table.
However, you only have one example. It's a lot more readable to do it this way:
| Scenario Outline: A User cannot create a bundle due to authorisation | |
| Given there is a <role> user | |
| When the <role> logs in | |
| Then the logged in user cannot create a bundle | |
| Examples: bundles | |
| | role | | |
| | Viewer | | |
| Scenario: A User cannot create a bundle due to authorisation | |
| Given there is a Viewer user | |
| When the Viewer logs in | |
| Then the logged in user cannot create a bundle |
It's clear what we are testing, who the user is and what they are trying to do, and we use fewer lines. It all reads better – you don't need to go to the table to check what the <role> parameter will be. There is no reason to "replicate" the table from the scenario outline which has multiple different roles – I see no benefit in doing that. It just makes the code clunkier and more difficult to read. If you can show me a benefit, I can definitely take it into consideration.
functional_tests/steps/bundles.py
Outdated
|
|
||
| @then("the logged in user cannot approve a bundle due to lack of pages") | ||
| def cannot_approve_bundle_edge_case(context: Context) -> None: | ||
| bundle_edit = f"Edit '{context.bundles[-1].name}'" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- This test fails here as it cannot find the edit link.
- This step should be split into at least two or three steps, e.g. "the user goes to the bundle view", "the user clicks approve", "the user is informed they cannot approve the bundle"
- You should check that the message "Cannot approve the bundle without any pages or datasets" is visible, not just that the page could not be saved, in case something else has prevented it from being saved - be explicit in your tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Resolved in 612ac1c
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Point 2 hasn't been addressed.
Consider your current scenario ("A user cannot approve a bundle due to lack of pages or datasets"). After your initial setup with the Given + And statements, you have essentially just two steps:
When the <role> logs in
Then the logged in user cannot approve a bundle due to lack of pagesThis is a functional/UI test. We should be explicit in what the user is trying to do. Look at existing tests in the bundle.feature file, like this example:
dis-wagtail/functional_tests/features/bundle.feature
Lines 63 to 72 in 48622f3
| Scenario: A content editor cannot set a release calendar page to cancelled when it is in a bundle | |
| When the user manually creates a future release calendar page with a "Provisional" status | |
| And the user clicks the "Save Draft" button | |
| And the user navigates to the bundle creation page | |
| And the user enters a title | |
| And the user opens the release calendar page chooser | |
| And the user selects the existing release calendar page | |
| And the user clicks "Save as draft" | |
| And the user tries to set the release calendar page status to "Cancelled" | |
| Then the user sees a validation error preventing the cancellation because the page is in a bundle |
Notice how we clearly outline all of the steps the user is taking: what they are clicking, what they are putting in etc.. Your scenario doesn't do that. Some of your other scenarios suffer from the same problem.
MaciekBaron
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you Ann for replying to some of my comments. It makes it easier to review and verify that you have addressed the issues raised. Could you please continue replying to all of the comments so that we know that everything has been addressed.
functional_tests/steps/bundles.py
Outdated
| @then("the logged in user adds a Name to the bundle") | ||
| def add_name_to_bundle(context: Context) -> None: | ||
| bundle_name = "Bundle UI Test Bundle" | ||
| if "bundles" in context: | ||
| bundle_name = context.bundles[-1].name | ||
| context.page.get_by_role("textbox", name="Name*").fill(bundle_name) | ||
|
|
||
|
|
||
| @then("the logged in user adds a Release Calendar page to the bundle") | ||
| def add_release_calendar_to_bundle(context: Context) -> None: | ||
| context.page.get_by_role("button", name="Choose Release Calendar page").click() | ||
| context.page.get_by_role("textbox", name="Search term").fill(context.release_calendar_page.title) | ||
| context.page.get_by_role("row", name=context.release_calendar_page.title).get_by_role("link").click() | ||
|
|
||
|
|
||
| @then("the logged in user add a schedule date to the bundle") | ||
| def add_schedule_date(context: Context) -> None: | ||
| now = (datetime.now() + timedelta(hours=4)).strftime("%Y-%m-%d %H:%M") | ||
| context.page.get_by_role("textbox", name="or Publication date").fill(now) | ||
|
|
||
|
|
||
| @then("the logged in user goes to the bundle page") | ||
| def go_to_bundle_page(context: Context) -> None: | ||
| context.page.get_by_role("link", name="Bundles", exact=True).click() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All of these shouldn't be @then statements but either @when or @step statements.
Please review all of your other statements because a lot of them suffer from the same issue. See my other comment which mentions the Gherkin documentation.
Additionally, there's a small typo on line 633 (should be "adds" not "add).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Resolved by
ba796fb
| When the <role> logs in | ||
| Then the logged in user goes to the bundle page | ||
| And the logged in user can find the bundle | ||
| And the <role> can preview bundle |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are not using When/Then correctly.
When specifies the action performed by a user that triggers the behaviour to be tested
Then specifies the outcome or expected result of the action
Please consult the Gherkin documentation to get a better understanding of how to structure your tests and what these terms mean.
This mistake is repeated in your other tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved in 8ae5c18
functional_tests/steps/bundles.py
Outdated
| context.page.get_by_role("textbox", name="Search term").fill(context.bundles[-1].name) | ||
| context.page.goto(context.base_url + "/admin/bundle/?q=" + context.bundles[-1].name) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't make sense. You fill in the search term and then force the browser to go to that URL.
You should instead be clicking the search button so that the search gets triggered. You make the same mistake on line 572.
Moreover - and I've highlighted this several times now in other places - you do not make any assertions here.
If this step is supposed to check that "the logged in user can find the bundle", what exactly are you doing to check that? You essentially just go to that URL and that's it. The URL could be wrong, it could return an error - many things can happen, but you're not checking for anything here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
resolved by 79810cc
I agree this is confusing
its was 'clingon' from playwright codegen
it should be the success search results expects
I need to make this viewer friendly
| @then("the logged in user can create a bundle") | ||
| def add_bundle_details(context: Context) -> None: | ||
| expect(context.page.get_by_role("link", name="Add bundle")).to_be_visible() | ||
| context.page.get_by_role("link", name="Add bundle").click() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is not clear why do you click on the button here. You already checked it is visible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Resolved by
ba796fb
it has now been split down to two steps
My logic ....
If a 'viewer' ran the success scenario it would fail at this point and is necessary to justify the exception
|
@AWitcherONS in case you are wondering why your tests are failing, we have upgraded behave to the latest version and it is case sensitive when it comes to step definitions. As you are mixing cases, it fails to find them. Please ensure consistent casing and to run the tests locally before pushing. |
|
Resolved by |
What is the context of this PR?
Bundles User Interface Journey from creation to Approval
Testing Strategy
User/Auth
Bundle:- volume, and contents
The feature tests should be completed in a timely manner avoiding duplication where possible
role, function (Search(volume is significant), Create, Edit/insert Edit/remove, send to moderation, preview, approve)
for each of these function what text can be seen, what fields to edit(insert/delete), what actions (save etc)
what happens when mandatory fields fail validations on actions
/
How to review
The Python code should comply with current standards
The feature tests run without failures and do not impact on run time
Follow-up Actions
A Viewer cannot use the Search in Bundles menu there isn't an error but no know bundles are not found
review on approval Authorisation of bundles