Skip to content

Conversation

gianick
Copy link
Contributor

@gianick gianick commented Aug 22, 2025

Fixed tests with outdated, redundant etc tests.

  • Fixed minor typo postcodedNonUkAddress
  • ReasonForRequestingRepaymentControllerSpec: removed redundant tests brought over from Acceptance Tests
  • HavePillar2TopUpTaxIdViewSpec: fixed incorrect testing of page title and H1 on
  • ContactNfmByPhoneViewSpec: fixed incorrect tests for title and H1
  • FilingMemberCheckYourAnswersViewSpec: fixed incorrect tests for title and H1
  • IsNFMUKBasedViewSpec: fixed incorrect tests for title and H1
  • NfmEmailAddressViewSpec: fixed incorrect tests for title and H1
  • NominateFilingMemberYesNoViewSpec: improved tests for radio buttons
  • RegistrationConfirmationViewSpec: improved/reduced usage of Jsoup selectors
  • RegistrationFailedNfmView: fixed page title not matching H1. Improved/reduced usage of jsoup selectors
  • UpeRegisteredAddressViewSpec: used a pagetitle builder method for page with Org name not appearing on page title
  • UPERegisteredInUKConfirmationViewSpec: fixed incorrect tests for title/h1 and improved tests for radio buttons
  • BankAccountDetailsViewSpec: removed redundant tests brought over from Acceptance Tests
  • RepaymentsConfirmationViewSpec
  • Introduced the usage of title generator methods for views with a company name or contact name (these do not appear on the title)
  • RfmRegisteredAddressViewSpec: used title generator
  • GroupAccountingPeriodViewSpec: removed redundant tests brought over from Acceptance Tests
  • improved testing for Print this Page elements
  • removed instances of must include from view specs
  • SecondaryContactEmailViewSpec: improved tests for form (xss, long input, empty)
  • SubCheckYourAnswersViewSpec: removed redundand tests brought over from Acceptance Tests
  • Moved Agent test to the Agent section in test/views/HomepageViewSpec.scala (PR [PIL-2321] - redirect Top Banner Link to DashboardController #516)
  • Replaced must include() with mustBe (PR PIL-2722 DEV Add content testing to unit tests 3 #507 & PR PIL-2173: Move commented acceptance test scenarios to frontend from branch PIL-2172 #499)
  • Removed duplicate vals, duplicate tests and redundant tests from views.repayments.RepaymentsConfirmationViewSpec
  • Added missing lazy to val declarations at the top of Specs

@platops-pr-bot
Copy link

@gianick gianick force-pushed the NOJIRA-fix-tests branch 5 times, most recently from de880ae to 468a0b9 Compare August 22, 2025 11:41
@platops-pr-bot
Copy link

@gianick gianick force-pushed the NOJIRA-fix-tests branch 3 times, most recently from 3421c01 to b203805 Compare September 10, 2025 09:01
@platops-pr-bot
Copy link

@platops-pr-bot
Copy link

@gianick gianick force-pushed the NOJIRA-fix-tests branch 3 times, most recently from ef52f17 to cd4e145 Compare October 16, 2025 11:00
@platops-pr-bot
Copy link

@gianick gianick marked this pull request as ready for review October 17, 2025 13:27
@gianick gianick force-pushed the NOJIRA-fix-tests branch 2 times, most recently from 65a1fb9 to 22655af Compare October 17, 2025 14:39
lazy val formProvider: ContactNfmByPhoneFormProvider = new ContactNfmByPhoneFormProvider
lazy val page: ContactNfmByPhoneView = inject[ContactNfmByPhoneView]
lazy val username: String = "John Doe"
lazy val pageTitle: String = "Can we contact by telephone" // FIXME: telephone?
Copy link
Contributor

Choose a reason for hiding this comment

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

telephone sounds good to me. Why is this a fixme?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah I see, seems it was somehow missed here:
https://jira.tools.tax.service.gov.uk/browse/PIL-2335

Maybe worth raising a ticket as it's a functional change

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well spotted. Thanks. This is not being used. Will remove it.

).toString()
)
lazy val page: FilingMemberCheckYourAnswersView = inject[FilingMemberCheckYourAnswersView]
lazy val pageTitle: String = "Check your answers for filing member details"
Copy link
Contributor

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 use this in the tests below

}
}

"when form has validation errors" should {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are we removing these tests?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These are tests that were brought over from the Acceptance Tests and they are redundant because we arleady have the following tests in this spec:

  • "when form is submitted with missing values"
  • "when form is submitted with values exceeding maximum length"
  • "when form is submitted with special characters"

Also the implementation of the BARS-related tests is wrong. A Form[BankAccountDetails] is created with `validForm.withError("accountNumber", "repayments.bankAccountDetails.error.accountNumber"), then passed to the view and then we test if the view has this error message.

Copy link
Contributor

@dc-smith dc-smith left a comment

Choose a reason for hiding this comment

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

Mostly looks good, also interested about the removal of some of these tests.

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