Manchester | 26-ITP-Sep | Salah Alsabhi | Sprint 3 | Stretch coursework by salahalsabhi · Pull Request #1751 · CodeYourFuture/Module-Structuring-and-Testing-Data

salahalsabhi · 2026-10-23T19:57:33Z

Learners, PR Template

Self checklist

I have titled my PR with Region | Cohort | FirstName LastName | Sprint | Assignment Title
My changes meet the requirements of the task
I have tested my changes
My changes follow the style guide

Task code

CYF-1065

Changelist

Implemented full password validation rules with tests
Added credit card validator (isValidCardNumber) with tests

Questions

No questions.

cjyuan

Functions are correctly implemented. Good job.

cjyuan · 2026-10-24T20:20:18Z

+test("rejects non-digit characters", () => {
+  expect(isValidCardNumber("a92332119c011112")).toEqual(false);
+});
+
+test("rejects all same digits", () => {
+  expect(isValidCardNumber("4444444444444444")).toEqual(false);
+});
+
+test("rejects sum of 16 or less", () => {
+  expect(isValidCardNumber("1111111111111110")).toEqual(false);
+});
+
+test("rejects odd final digit", () => {
+  expect(isValidCardNumber("6666666666666661")).toEqual(false);
+});


Could also test card numbers that do not have exactly 16 digits.

Some of the test descriptions are not yet properly phrased. Could you rephrase them? A common pattern you could try is "should ____________ when _________________".

cjyuan · 2026-10-24T20:21:37Z

+const previousPasswords = ["password", "12345", "qwerty"];
+
 export function isValidPassword(password) {


For higher flexibility, we could design the function to accept previousPasswords as an optional second parameter (with a default value).

cjyuan · 2026-10-24T20:24:11Z

+  expect(isValidPassword("ABC1!")).toEqual(false);
+});
+
+test("rejects password without number", () => {


Note: The term digit refers to a number in the range 0-9. A number may contain one or more digits.

salahalsabhi · 2026-10-25T06:50:21Z

Thanks for the feedback. I’ve updated the test names, added length checks for card numbers, and made previousPasswords an optional second parameter with a default.

cjyuan

To test a function that could return false for multiple reasons, we should choose an input (argument) that satisfies all conditions except the one we're targeting.
This way, if the function returns false, we can confidently attribute the result to that specific condition.

To illustrate, consider this example:

  test("returns false when the password has fewer than 5 characters", () => {
    expect(isValidPassword("1234")).toBe(false);
  });

The test case "1234" is a poor choice because it violates multiple conditions, not just the minimum length requirement.
Even if the function incorrectly checks the password length, this test could still pass because the function returns false for one of the other reasons.

A better test case would be "Ab1!" because it satisfies all conditions except the minimum length requirement.

Could you review each test case to ensure it isolates the specific condition being tested? Make sure the input satisfies all the other conditions and violates only the target condition, so that if the test passes, you can be confident that the specific condition is being checked correctly.

cjyuan · 2026-10-25T12:13:52Z

+const defaultPreviousPasswords = ["password", "12345", "qwerty"];
+
+export function isValidPassword(
+  password,
+  previousPasswords = defaultPreviousPasswords
+) {


Why set these as default previous passwords? If the function is used in an application, how would these default passwords affect the application? If they are for testing purpose, why not declare them in the test script.

salahalsabhi · 2026-10-25T13:08:49Z

Updated tests so each invalid case only breaks one rule, and moved previous passwords out of the function default into the tests (default is now []).

cjyuan

Some of the test cases in card-validator.test.js also violate more than one conditions. Could you fix them?

cjyuan · 2026-10-25T13:44:52Z

+test("should reject a previous password", () => {
+  expect(isValidPassword("password", previousPasswords)).toEqual(false);
+});


The function could also return false because "password" violates several other conditions.

And this test is very similar to the next test (lines 50-52). You could consider grouping both tests into one category.

salahalsabhi · 2026-10-25T18:31:35Z

Isolated each card invalid case to a single rule, and reduced previous-password coverage to one test that only fails via the previousPasswords list.

cjyuan

Changes look good.

cjyuan · 2026-10-25T18:56:50Z

-const previousPasswords = ["password", "12345", "qwerty"];
-


Larger array of previous passwords was a good for testing. It was just that all the previous passwords you chose were invalid.

salahalsabhi added 2 commits October 23, 2026 20:12

Implement password validator with full rules and tests

e3e959b

Add credit card validator with tests

aab021a

salahalsabhi added the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Oct 23, 2026

cjyuan reviewed Oct 24, 2026

View reviewed changes

cjyuan added Reviewed Volunteer to add when completing a review with trainee action still to take. and removed Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. labels Oct 24, 2026

Address review: test names, length cases, previousPasswords

bd6a8fb

salahalsabhi added the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Oct 25, 2026

cjyuan reviewed Oct 25, 2026

View reviewed changes

cjyuan removed the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Oct 25, 2026

Isolate password test cases; use empty default for previousPasswords

987378d

salahalsabhi added the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Oct 25, 2026

cjyuan reviewed Oct 25, 2026

View reviewed changes

cjyuan removed the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Oct 25, 2026

Isolate card tests; single previousPasswords test

e378e8d

salahalsabhi added the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Oct 25, 2026

cjyuan reviewed Oct 25, 2026

View reviewed changes

		const previousPasswords = ["password", "12345", "qwerty"];

		export function isValidPassword(password) {

Conversation

salahalsabhi commented Oct 23, 2026

Learners, PR Template

Task code

Changelist

Questions

Uh oh!

cjyuan left a comment

Choose a reason for hiding this comment

Uh oh!

cjyuan Oct 24, 2026

Choose a reason for hiding this comment

Uh oh!

cjyuan Oct 24, 2026

Choose a reason for hiding this comment

Uh oh!

cjyuan Oct 24, 2026

Choose a reason for hiding this comment

Uh oh!

salahalsabhi commented Oct 25, 2026

Uh oh!

cjyuan left a comment

Choose a reason for hiding this comment

Uh oh!

cjyuan Oct 25, 2026

Choose a reason for hiding this comment

Uh oh!

salahalsabhi commented Oct 25, 2026

Uh oh!

cjyuan left a comment

Choose a reason for hiding this comment

Uh oh!

cjyuan Oct 25, 2026

Choose a reason for hiding this comment

Uh oh!

salahalsabhi commented Oct 25, 2026

Uh oh!

cjyuan left a comment

Choose a reason for hiding this comment

Uh oh!

cjyuan Oct 25, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants