Skip to content

added implementation from unit test assignment#7

Open
lrassbach wants to merge 4 commits intomainfrom
Lucas_Rassbach_CodeReview
Open

added implementation from unit test assignment#7
lrassbach wants to merge 4 commits intomainfrom
Lucas_Rassbach_CodeReview

Conversation

@lrassbach
Copy link

No description provided.

@sarafarag sarafarag requested a review from GonGustavo June 15, 2023 18:41
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;

import org.junit.*;

Choose a reason for hiding this comment

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

"package org.junit does not exist" this might be causing the error for "cannot find symbol"

Choose a reason for hiding this comment

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

I added this to your StringManipulationTest. java to see if I could fix the problem and it seem too.

import static org.junit.jupiter.api.Assertions.*;

Causes the test cases to pass.

Copy link

@GonGustavo GonGustavo Jun 18, 2023

Choose a reason for hiding this comment

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

Screenshot 2023-06-17 at 6 39 17 PM

Once I got it to work on my end and passing the test cases. Here's a screenshot on what I got through the verify with Maven.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for reviewing. I'll make the proposed change.

@@ -0,0 +1,13 @@
import org.junit.runner.*;

Choose a reason for hiding this comment

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

This whole file is failing, maybe just condense it and add it to the file StringManipulationTest.java

Choose a reason for hiding this comment

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

When I got the other stuff to work and ran your TestRunner.java all it printed was "true".

Choose a reason for hiding this comment

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

Probably don't need this file then.

Copy link
Author

Choose a reason for hiding this comment

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

I'll remove the file. Thanks for reviewing.


@Test
public void testRestoreString1()
{

Choose a reason for hiding this comment

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

Is this suppose to be empty? I believe it should have a test case in it

Copy link
Author

Choose a reason for hiding this comment

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

It is not empty - there were just no changes made.

public String removeNthCharacter(int n, boolean maintainSpacing) {
return null;
if(n <= 0){
throw new IllegalArgumentException("n must be greater than or equal to 0");

Choose a reason for hiding this comment

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

Maybe give a message on my it throws and error.

Choose a reason for hiding this comment

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

I just realized that it does have a message in it.

public int count() {
return 0;
int tally = 0;
if(myString == null || myString.length() == 0){

Choose a reason for hiding this comment

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

I feel like this can be shorter for counting.

Copy link
Author

Choose a reason for hiding this comment

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

Can you elaborate?

ACBxBC added a commit that referenced this pull request Jun 17, 2023
…r whitepsaces in the #7 test case for restoreString.
@GonGustavo
Copy link

GonGustavo commented Jun 19, 2023

Also you need test cases for throwing the exceptions for each one in StringManipulation.java. Had the same issue in my code once I did that it passed the travis build.

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.

2 participants