Skip to content

Sheffield | ITP-Jan-26| Mona_Eltantawy | Sprint 3 | Sprint 3/ implement and testing data#1280

Open
Mona-Eltantawy wants to merge 9 commits intoCodeYourFuture:mainfrom
Mona-Eltantawy:Implement
Open

Sheffield | ITP-Jan-26| Mona_Eltantawy | Sprint 3 | Sprint 3/ implement and testing data#1280
Mona-Eltantawy wants to merge 9 commits intoCodeYourFuture:mainfrom
Mona-Eltantawy:Implement

Conversation

@Mona-Eltantawy
Copy link
Copy Markdown

@Mona-Eltantawy Mona-Eltantawy commented Mar 28, 2026

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

@Mona-Eltantawy Mona-Eltantawy added the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Mar 28, 2026
@github-actions

This comment has been minimized.

Sign in to view
@github-actions github-actions bot removed the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Mar 28, 2026
@github-actions

This comment has been minimized.

Sign in to view
@Mona-Eltantawy Mona-Eltantawy changed the title Sheffield | ITP-Jan-26| Mona_Eltantawy | Sprint3 | Sprint3/ implement and werite Sheffield | ITP-Jan-26| Mona_Eltantawy | Sprint 3 | Sprint 3/ implement and werite Mar 29, 2026
@Mona-Eltantawy Mona-Eltantawy added the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Mar 29, 2026
@Mona-Eltantawy Mona-Eltantawy mentioned this pull request Mar 29, 2026
Open
cjyuan
cjyuan reviewed Mar 29, 2026
View reviewed changes
Comment thread Sprint-3/1-implement-and-rewrite-tests/implement/1-get-angle-type.js Outdated
Comment thread Sprint-3/1-implement-and-rewrite-tests/implement/2-is-proper-fraction.js Outdated
Comment thread Sprint-3/1-implement-and-rewrite-tests/implement/3-get-card-value.js Outdated
Comment thread Sprint-3/1-implement-and-rewrite-tests/rewrite-tests-with-jest/1-get-angle-type.test.js Outdated
@cjyuan 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 Mar 29, 2026
@Mona-Eltantawy Mona-Eltantawy added the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Apr 14, 2026
@cjyuan
Copy link
Copy Markdown
Contributor

cjyuan commented Apr 14, 2026

No new commits on this branch on GitHub yet.

@cjyuan cjyuan removed the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Apr 14, 2026
@github-actions

This comment has been minimized.

Sign in to view
@cjyuan
Copy link
Copy Markdown
Contributor

cjyuan commented Apr 15, 2026

You have another PR on the same exercise. You need to close the other one or else this PR won't pass the validation.

#1275

@Mona-Eltantawy
Copy link
Copy Markdown
Author

Mona-Eltantawy commented Apr 18, 2026

You have another PR on the same exercise. You need to close the other one or else this PR won't pass the validation.

#1275

I closed the other one can you recheck please

@Mona-Eltantawy Mona-Eltantawy added the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Apr 18, 2026
@github-actions

This comment has been minimized.

Sign in to view
@github-actions github-actions bot removed the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Apr 18, 2026
@Mona-Eltantawy Mona-Eltantawy changed the title Sheffield | ITP-Jan-26| Mona_Eltantawy | Sprint 3 | Sprint 3/ implement and werite Sheffield | ITP-Jan-26| Mona_Eltantawy | Sprint 3 | Sprint 3/ implement and write Apr 18, 2026
@github-actions

This comment has been minimized.

Sign in to view
@Mona-Eltantawy Mona-Eltantawy changed the title Sheffield | ITP-Jan-26| Mona_Eltantawy | Sprint 3 | Sprint 3/ implement and write Sheffield | ITP-Jan-26| Mona_Eltantawy | Sprint 3 | Sprint 3/ implement and rewrite Apr 18, 2026
@github-actions

This comment has been minimized.

Sign in to view
@Mona-Eltantawy Mona-Eltantawy changed the title Sheffield | ITP-Jan-26| Mona_Eltantawy | Sprint 3 | Sprint 3/ implement and rewrite Sheffield | ITP-Jan-26| Mona_Eltantawy | Sprint 3 | Sprint 3/ implement and testing data Apr 18, 2026
@github-actions

This comment has been minimized.

Sign in to view
@Mona-Eltantawy Mona-Eltantawy added the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Apr 18, 2026
cjyuan
cjyuan reviewed Apr 18, 2026
View reviewed changes
Comment thread Sprint-3/1-implement-and-rewrite-tests/implement/3-get-card-value.js Outdated
Comment thread Sprint-3/1-implement-and-rewrite-tests/rewrite-tests-with-jest/1-get-angle-type.test.js
Comment on lines +53 to +59
describe("Invalid angle", () => {
test("should return 'Invalid angle' when (angle <= 0 or angle > 360)", () => {
expect(getAngleType(0)).toBe("Invalid angle");
expect(getAngleType(400)).toBe("Invalid angle");
expect(getAngleType(-10)).toBe("Invalid angle");
});
});
Copy link
Copy Markdown
Contributor

@cjyuan cjyuan Apr 18, 2026

Choose a reason for hiding this comment

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

  • 360 is also an invalid angle. The test description could include it too.

  • Why not test both boundary cases?

Copy link
Copy Markdown
Author

@Mona-Eltantawy Mona-Eltantawy Apr 18, 2026

Choose a reason for hiding this comment

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

I added the boundary cases

Copy link
Copy Markdown
Contributor

@cjyuan cjyuan Apr 18, 2026

Choose a reason for hiding this comment

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

You didn't update the test description.

Comment thread Sprint-3/1-implement-and-rewrite-tests/rewrite-tests-with-jest/2-is-proper-fraction.test.js
Comment on lines +31 to +43
test("should return false when numerator is negative", () => {
expect(isProperFraction(-2, 4)).toEqual(false);
});

// negative denominator
test("should return false when denominator is negative", () => {
expect(isProperFraction(5, -4)).toEqual(false);
});

// both negative
test("should return false when both numerator and denominator are negative", () => {
expect(isProperFraction(-3, -5)).toEqual(false);
});
Copy link
Copy Markdown
Contributor

@cjyuan cjyuan Apr 18, 2026

Choose a reason for hiding this comment

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

Can your function pass all these tests?

We can use pseudo-code and notations like abs(...) or | ... | in the test descriptions to more
concisely describe the conditions (the "when" part).

Copy link
Copy Markdown
Author

@Mona-Eltantawy Mona-Eltantawy Apr 18, 2026

Choose a reason for hiding this comment

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

I fixed the function to pass all the tests

Comment thread Sprint-3/1-implement-and-rewrite-tests/rewrite-tests-with-jest/3-get-card-value.test.js
Copy link
Copy Markdown
Contributor

@cjyuan cjyuan Apr 18, 2026

Choose a reason for hiding this comment

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

This file should contain the tests for getCardValue().

Copy link
Copy Markdown
Author

@Mona-Eltantawy Mona-Eltantawy Apr 18, 2026

Choose a reason for hiding this comment

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

done

@cjyuan cjyuan removed the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Apr 18, 2026
@Mona-Eltantawy Mona-Eltantawy added the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Apr 18, 2026
cjyuan
cjyuan reviewed Apr 18, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

@cjyuan cjyuan left a comment

Choose a reason for hiding this comment

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

Looks good.

@cjyuan cjyuan added Complete Volunteer to add when work is complete and all review comments have been addressed. and removed Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. Reviewed Volunteer to add when completing a review with trainee action still to take. labels Apr 18, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cjyuan cjyuan cjyuan left review comments

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

Complete Volunteer to add when work is complete and all review comments have been addressed.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@Mona-Eltantawy @cjyuan