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

Migrated tests and pages from blueprint and removed example tests #2

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

Andyg79
Copy link

@Andyg79 Andyg79 commented Feb 5, 2025

Description

Context

Type of changes

  • Refactoring (non-breaking change)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would change existing functionality)
  • Bug fix (non-breaking change which fixes an issue)

Checklist

  • I am familiar with the contributing guidelines
  • I have followed the code style of the project
  • I have added tests to cover my changes (where appropriate)
  • I have updated the documentation accordingly
  • This PR is a result of pair or mob programming

Sensitive Information Declaration

To ensure the utmost confidentiality and protect your and others privacy, we kindly ask you to NOT including PII (Personal Identifiable Information) / PID (Personal Identifiable Data) or any other sensitive data in this PR (Pull Request) and the codebase changes. We will remove any PR that do contain any sensitive information. We really appreciate your cooperation in this matter.

  • I confirm that neither PII/PID nor sensitive data are included in this PR and the codebase changes.

@Andyg79 Andyg79 self-assigned this Feb 5, 2025
pages/screening_subject_search_page.py Outdated Show resolved Hide resolved
tests/test_call_and_recall_page.py Outdated Show resolved Hide resolved
homepage.click_refresh_alerts_link()
# Verify that the 'last updated' timestamp matches the current date and time
(expect(page.locator("form[name=\"refreshCockpit\"]")).to_contain_text
("Refresh alerts (last updated :" + DateTimeUtils.current_datetime()))
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would amend this to f"Refresh alerts (last updated : {DateTimeUtils.current_datetime()})"


def test_login_to_bcss_with_invalid_username(page: Page) -> None:
username = "BCSSZZZ"
password = "changeme"
Copy link
Collaborator

Choose a reason for hiding this comment

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

We need to remove this secret and pass it into the code

expect(page.locator("b")).to_contain_text(report_timestamp)

# Open an appointment record from the report
page.get_by_role("link", name="934 9288").click()
Copy link
Collaborator

Choose a reason for hiding this comment

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

This looks like an NHS number so we'd need to change the locator to be more generic

def login_as_user_bcss401(self):
"""Logs in to bcss as the test user 'BCSS401'"""
self.username.fill("BCSS401")
self.password.fill("changeme")
Copy link
Collaborator

Choose a reason for hiding this comment

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

We need to remove this as it's a secret and pass it in - it might also be worth us importing the user util from the blueprint to manage the account access

Copy link
Collaborator

Choose a reason for hiding this comment

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

In the bcss-playwright repo I have removed the login_as_user function and pass in the login info from an environmental variable in the test itself, this can either be loaded in from a dotenv file (.env) or if run in a pipeline have the values injected from a secret.

it looks like this:

load_dotenv()
expect.set_options(timeout=30_000)


@pytest.fixture(scope="function", autouse=True)
def before_each(page: Page):
    # This will run before every other job and log in to the homepage.
    username = environ.get("BCSS_USERNAME")
    password = environ.get("BCSS_PASSWORD")
    login_page = BcssLoginPage(page)
    login_page.login(username, password)

Copy link
Collaborator

Choose a reason for hiding this comment

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

That make sense for the password and I very much like the way that's been done because it's nice and clean, but passing in the username in the same fashion could be problematic once we've got a bunch of tests focusing on different areas of the system, and because BCSS is complex we often require multiple users to achieve the end result. I'd be inclined to do this logic for passwords but maintain the users in the code directly, more from a maintenance standpoint.

tests/test_call_and_recall_page.py Show resolved Hide resolved
Victor Soares and others added 5 commits February 12, 2025 11:13
Changed the dropdown option value numbers in the functions to class variables.
…ub.com:NHSDigital/bcss-playwright into feature/migrate_tests_and_utils_from_blueprint
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.

4 participants