Exercise 6: the skipped tests don't line up with what you are expected to do

[Jacksonbryan28]

Prerequisites

• I have thoroughly read and understand The Odin Project Contributing Guide
• I have searched the existing issues to ensure this improvement hasn't been suggested before (please have a look through our open issues list to make sure)
• The title of this issue follows the <Location of the improvement>: <Brief description of the improvement> format, e.g. Exercises: Add exercise on XYZ
• (Optional) I am interested in working on this issue and would like to be assigned to it

Improvement Description

In lesson 6 there are tests that are skipped that don't line up with what is asked in the readme.

The .skiped tests include giving an error if the output is a negative number.

test.skip('returns ERROR with negative numbers', () => {
expect(repeatString('goodbye', -1)).toEqual('ERROR');
});
test.skip('repeats the string a random amount of times', function () {
/* The number is generated by using Math.random to get a value from between
0 to 1, when this is multiplied by 1000 and rounded down with Math.floor it
equals a number between 0 to 999 (this number will change everytime you run
the test).

  DO NOT use Math.floor(Math.random() * 1000) in your code, 
  this test generates a random number, then passes it into your code with a function parameter.
  If this doesn't make sense, you should go read about functions here:
  https://www.theodinproject.com/paths/foundations/courses/foundations/lessons/fundamentals-part-3 */
const number = Math.floor(Math.random() * 1000);
expect(repeatString('hey', number)).toBe('hey'.repeat(number));

});
test.skip('works with blank strings', () => {
expect(repeatString('', 10)).toEqual('');
});

The readme does not mention this anywhere

Exercise 06 - repeatString

Write a function that simply repeats the string a given number of times:

repeatString("hey", 3); // returns 'heyheyhey'

This function will take two arguments, string and num.

Use loops to implement repeatString rather than using the builtin String.prototype.repeat which has the same behaviour.

Note: The exercises after this one will not have arguments provided as this one does - you will need to provide them yourself from now on. So read each exercise's README carefully to see what kinds of arguments will be expected.

Hints

• Take note of the above function call- how exactly is it being called?

• Create a variable to hold the string you're going to return, create a loop that repeats the given number of times and add the given string to the result on each loop.

I made another ticket about skips not being introduced into the readme as well.

Acceptance Criteria

[ ] remove skips that don't align with the readme
or
[ ] make it so the readme reflects what is expected from the test

(Optional) Additional Comments

No response

[CouchofTomato]

@TheOdinProject/javascript Can someone take a look please

[mao-sz]

Though I'm not a maint anymore, my 2c is that this probably doesn't need anything done.
The repo README instruct about how all tests need to pass simultaneously, and you unskip them one by one, and this is repeated in exercise 3 as that's the first exercise with multiple test cases.

The exercises' READMEs are just the briefs for the exercises but it's made clear from the instructions that the tests will have the specifics that need implementing. IMO it's sufficient to be told to repeat a string N times and handling negative numbers wouldn't really be a point of confusion for me. When I unskip the tests one by one after passing each, I'd naturally be instructed what to do next.

[Jacksonbryan28]

I would disagree with this. The readme tells me what is required in all the exercises I've completed so far. This is the only lesson where the readme doesn't match up with the tests. Either the readme needs to include the scope of all the tests, or the tests that aren't called out in the readme should be removed.

My opinion on the scope of this lesson - I'm a beginner going through foundations, I don't think I would know how to do the tests that are not mentioned in the readme, so I think they should be removed. Maybe someone with more experience could make that call though.

[Eduardo06sp]

Personally (from the perspective of a learner), I don't see any issues with the way it is already. I think that moving into testing for edge-cases is really important.

In the wider scope of things, these tests can help prepare you for testing your own projects, which will get more and more advanced.

By introducing edge-case tests that you may not have been thinking about before, it (possibly) forces you to realize that you should test for things you shouldn't do, not just things you should do.

The first few exercises are generally a lot simpler and "hand-holdey", then they get more difficult after that, which I'm a huge fan of.

[PaavanaBV]

Hello! I would like to work on resolving this issue.

I agree that the simplest fix to align with the README is to remove the misaligned test.skip blocks from repeatString.spec.js. This will ensure the exercise focuses on the core functionality without introducing confusing, undocumented edge cases.

I have already forked the repository, installed dependencies, and am ready to implement and validate the fix locally.

Could you please assign this issue to me?

Thank you!

[mao-sz]

@Jacksonbryan28 let's add to the exercise README a simple "If the repeat count is a negative number, return the string 'ERROR' instead."
Let me know if you'd still like to be assigned to this, then I can do so.

[mao-sz]

@PaavanaBV assigning you instead. Please see my previous message for the desired change.