Skip to content

add error-handling exercise #2732

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

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

Conversation

A-O-Emmanuel
Copy link

Implemented the Error Handling practice exercise with solution, tests, and documentation

This comment has been minimized.

@github-actions github-actions bot closed this Jul 24, 2025
@SleeplessByte SleeplessByte reopened this Jul 25, 2025
@SleeplessByte

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as off-topic.

This comment was marked as outdated.

Copy link
Member

Choose a reason for hiding this comment

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

The contents are missing here, that's why the CI is failing!

Copy link
Member

Choose a reason for hiding this comment

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

All the other exercises have a "stub" (initial code) that aids with implementation. Can you check the other concept exercises how we do that and add it here? It probably needs to define processString and merely raise an error to implement the function.

Copy link
Author

Choose a reason for hiding this comment

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

Alight, I'll do that

Comment on lines +22 to +35
console.log = jest.fn();

try {
processString('');
} catch {
/*
intentionally left empty,
I expext this call to throw,
but only care about verifying that the finally block is executed
and clean up message logged.
*/
}

expect(console.log).toHaveBeenCalledWith('Resource cleaned up');
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this will work with the Browser Test Runner, but the idea is not wrong. I can help come up with an easy alternative to fix this.

Copy link
Author

Choose a reason for hiding this comment

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

okay, thank you

@@ -0,0 +1,15 @@
{
"authors": ["Obinna Obi-Akwari"],
Copy link
Member

Choose a reason for hiding this comment

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

This should be your GitHub user name, not what I’m guessing is your actual name.

Copy link
Author

Choose a reason for hiding this comment

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

okay, i'll add it, thank you

Copy link
Member

@BNAndras BNAndras left a comment

Choose a reason for hiding this comment

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

I’m not a track maintainer here, just a friendly maintainer from elsewhere on Exercism. I made some more general comments, but whatever SleeplessBytes and Cool-Kat say is the final word. :)

Copy link
Member

Choose a reason for hiding this comment

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

Generally, tests after the first one should be skipped by default. The online test runner will run all the tests, but that allows students working offline to practice TDD and solve tests one at a time.

You’re using it here, but the convention on this track is to use test and the corresponding xtest. See https://github.com/exercism/javascript/blob/main/exercises/practice/leap/leap.spec.js as an example.

Copy link
Member

Choose a reason for hiding this comment

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

Also would it make sense to use custom exceptions here? Then you don’t need to check both the type and message.

Copy link
Author

Choose a reason for hiding this comment

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

Any advice and recommendation is good for me, thank you I will look at everything you said

expect(() => processString('')).toThrow('EmptyStringError');
});

it('always logs cleanup message', () => {
Copy link
Member

Choose a reason for hiding this comment

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

This test checks if a cleanup message is logged after an empty string is provided. That isn’t the same as checking if a cleanup message is always logged.

@@ -0,0 +1,7 @@
export const processString = (input) => {
//TODO: implement this
//should throw Error if input is not a string
Copy link
Member

Choose a reason for hiding this comment

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

This line isn’t correct. The test suite is looking for a specific error type.

export const processString = (input) => {
//TODO: implement this
//should throw Error if input is not a string
//should throw Error if input is an empty string
Copy link
Member

Choose a reason for hiding this comment

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

This implies any Error can be thrown with any message, but that’s not true.

//TODO: implement this
//should throw Error if input is not a string
//should throw Error if input is an empty string
//should return the uppercase version of the string if valid
Copy link
Member

Choose a reason for hiding this comment

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

What’s considered valid here? This line implies by nearness that not all strings are valid, but it’s not clear what is valid.

//should throw Error if input is not a string
//should throw Error if input is an empty string
//should return the uppercase version of the string if valid
//should use finally to log cleanup message
Copy link
Member

Choose a reason for hiding this comment

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

Cleanup of what exactly?

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.

3 participants