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

CYF-ITP-South Africa | Rashaad Ebrahim | Module-Structuring-and-Testing-Data | Week 3 #225

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

Conversation

Rashaad-Ebrahim
Copy link

@Rashaad-Ebrahim Rashaad-Ebrahim commented Dec 14, 2024

Learners, PR Template

Self checklist

  • I have committed my files one by one, on purpose, and for a reason
  • I have titled my PR with COHORT_NAME | FIRST_NAME LAST_NAME | REPO_NAME | WEEK
  • I have tested my changes
  • My changes follow the style guide
  • My changes meet the requirements of this task

Changelist

Exercises completed and all necessary tests created.

Questions

On the is-prime.test.js, I created fours tests. Two related to testing prime numbers, and two related to testing non- prime numbers.

Of the two tests, the first test is to check if the function was working correctly. What I was trying to, was run the first test and if it failed, then only would the second test be run. The second test would then indicate where the error is coming from.

Is this even possible with Jest? I tried many different thing but to no avail. Any input?

What I wanted to to

@Rashaad-Ebrahim Rashaad-Ebrahim added the Needs Review Participant to add when requesting review label Dec 14, 2024
@shreefAhmedM shreefAhmedM added Reviewed Volunteer to add when completing a review and removed Needs Review Participant to add when requesting review labels Dec 14, 2024
@cjyuan
Copy link

cjyuan commented Dec 14, 2024

Each describe in Jest is carried out concurrently. As a result, it is difficult to ensure tests in two different describe's are performed in a particular order. Even within the same describe, it is not easy to specify "perform test B if test A fails".

I am not sure why you need short/long test in your 'is-prime.test.js`. If a short test fails, what's the reason to run the longer version?

Here is one possible way to perform test B if test A fails:

  test("Perform test B if test A failed", () => {
    try {
      // Test A
      expect(true).toBe(false);  // Simulate a failed test
    } catch (error) {
      // Test B
      expect(...).toBe(...);

      // Nested "test" is not allowed; we cannot call test() inside a test().
      // test("Test B", () => { expect(...).toBe(...); });
    }
  });

@cjyuan
Copy link

cjyuan commented Dec 14, 2024

@shreefAhmedM I noticed that you have added the "Reviewed" label in this PR, but I cannot see any of your comment. Have you reviewed Rashaad code and gave feedback to Rashaad accordingly?

@Rashaad-Ebrahim
Copy link
Author

Thanks @cjyuan, I'll give that a try.

I know this isn't a great scenario to run a "short" and "long" test, the idea just came to me and I wanted to know if its possible in the event I ever want to use it in another application.

@cjyuan
Copy link

cjyuan commented Dec 14, 2024

@Rashaad-Ebrahim Does this PR still need any review?

@shreefAhmedM
Copy link

@shreefAhmedM I noticed that you have added the "Reviewed" label in this PR, but I cannot see any of your comment. Have you reviewed Rashaad code and gave feedback to Rashaad accordingly?
Im very sorry about that its happened by mistake I was trying to implement some changes based on your review

@Rashaad-Ebrahim
Copy link
Author

@cjyuan I do still ned my PR reviewed.

@Rashaad-Ebrahim Rashaad-Ebrahim added Needs Review Participant to add when requesting review and removed Reviewed Volunteer to add when completing a review labels Dec 16, 2024
Copy link

@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.

Code is well written and tests are very comprehensive!
I left some comments and suggestions.

The function in get-card-value.js can miss some invalid cases.

Sprint-3/implement/get-angle-type.js Outdated Show resolved Hide resolved
Sprint-3/implement/get-angle-type.test.js Outdated Show resolved Hide resolved
Sprint-3/implement/get-card-value.js Outdated Show resolved Hide resolved
Sprint-3/implement/get-card-value.test.js Show resolved Hide resolved
Sprint-3/implement/rotate-char.js Show resolved Hide resolved
Sprint-3/revise/implement/card-validator.js Outdated Show resolved Hide resolved
Sprint-3/revise/implement/get-ordinal-number.js Outdated Show resolved Hide resolved
Sprint-3/revise/implement/is-prime.test.js Show resolved Hide resolved
Sprint-3/revise/investigate/find.js Show resolved Hide resolved
Sprint-3/revise/investigate/find.js Show resolved Hide resolved
@cjyuan cjyuan added Reviewed Volunteer to add when completing a review and removed Needs Review Participant to add when requesting review labels Dec 16, 2024
@Rashaad-Ebrahim
Copy link
Author

Suggested changes have been implemented.

@Rashaad-Ebrahim Rashaad-Ebrahim added Needs Review Participant to add when requesting review and removed Reviewed Volunteer to add when completing a review labels Dec 16, 2024
Copy link

@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.

Code looks good. Just few more suggestions.

Sprint-3/implement/get-card-value.js Outdated Show resolved Hide resolved
Sprint-3/implement/get-card-value.js Outdated Show resolved Hide resolved
Sprint-3/revise/implement/get-ordinal-number.js Outdated Show resolved Hide resolved
@cjyuan cjyuan added Reviewed Volunteer to add when completing a review and removed Needs Review Participant to add when requesting review labels Dec 16, 2024
@Rashaad-Ebrahim
Copy link
Author

Changes implemented.

@Rashaad-Ebrahim Rashaad-Ebrahim added Needs Review Participant to add when requesting review and removed Reviewed Volunteer to add when completing a review labels Dec 16, 2024
@cjyuan
Copy link

cjyuan commented Dec 17, 2024

You can mark your PR as "completed" any time you think you have made all the changes. You don't have to request for review for every change.

@cjyuan cjyuan added Reviewed Volunteer to add when completing a review and removed Needs Review Participant to add when requesting review labels Dec 17, 2024
@Rashaad-Ebrahim Rashaad-Ebrahim added the Complete Participant to add when work is complete and review comments have been addressed label Dec 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Complete Participant to add when work is complete and review comments have been addressed Reviewed Volunteer to add when completing a review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[TECH ED] Complete Sprint 3 exercises
3 participants